Re: [PATCH v3] gitk: Allow commit editing

[Michal Sojka <sojka@os.inf.tu-dresden.de>]

On Thu, 25 Aug 2011, Jeff King wrote:
> On Fri, Aug 19, 2011 at 02:25:53PM +0200, Michal Sojka wrote:
> 
> > +    if {[exec git branch -r --contains=$rowmenuid] ne {}} {
> > +	if {![confirm_popup [mc "The commit you are going to edit appears 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 }
> 
> We frown on using porcelain like "git branch" here, because it was not
> meant to be scriptable (i.e., its behavior may change in future
> releases).
> 
> As I mentioned elsewhere, I think you really want to see whether the
> commit is referenced by any other ref, not just branches; if it is, then
> the rebase is potentially problematic. You can do that with something
> like:
> 
>   us=`git symbolic-ref HEAD`
>   git for-each-ref --format='%(refname)' |
>   while read ref; do
>     test "$ref" = $us" && continue
>     echo "^$ref"
>   done |
>   git rev-list HEAD --stdin
> 
> That will give a list of commits found only on HEAD and nowhere else
> (i.e., those which are safe to rebase). If your commit is among them,
> then it's safe.

I implemented something like you ontlined above (see below). Instead of
rev-listing HEAD, I use the commit id to be edited. This way I don't
have to find the commit in the returned list, but I only check whether
the list is empty or not.

Additionally, besides skiping HEAD ref, I also skip refs/stash. Rebasing
before stash should not (I hope) cause any problems and therefore it is
not necessary to warn the user.

> Speaking of which, notice that I used HEAD here. What happens with your
> patch if I do:
> 
>   $ git checkout foo
>   $ gitk bar
> 
> and select a commit to edit that is not in "foo"?

I added a check for this. If this is detected, error message is
displayed and no edit is possible.

The other changes in this version are:
- added --no-autosquash to rebase invocation
- fixed indexes of menu entries following the new edit entry.

Regards,
-Michal

--8<---------------cut here---------------start------------->8---
I often use gitk to review patches before pushing them out and I would
like to have an easy way to edit commits. 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 the commit message or removing 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.

Besides editing the commit message, splitting of commits, as described
in git-rebase(1), is also supported.

The user is warned if the commit to be edited is contained in another
ref besides the current branch and the stash (e.g. in a remote
branch). Additionally, error box is displayed if the user attempts to
edit a commit not contained in the current branch.

Signed-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>
---
 gitk-git/gitk |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..0968efa 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}
@@ -8428,18 +8429,18 @@ proc rowmenu {x y id} {
     if {$id ne $nullid && $id ne $nullid2} {
 	set menu $rowctxmenu
 	if {$mainhead ne {}} {
-	    $menu entryconfigure 7 -label [mc "Reset %s branch to here" $mainhead] -state normal
+	    $menu entryconfigure 8 -label [mc "Reset %s branch to here" $mainhead] -state normal
 	} else {
-	    $menu entryconfigure 7 -label [mc "Detached head: can't reset" $mainhead] -state disabled
+	    $menu entryconfigure 8 -label [mc "Detached head: can't reset" $mainhead] -state disabled
 	}
 	if {[info exists markedid] && $markedid ne $id} {
-	    $menu entryconfigure 9 -state normal
 	    $menu entryconfigure 10 -state normal
 	    $menu entryconfigure 11 -state normal
+	    $menu entryconfigure 12 -state normal
 	} else {
-	    $menu entryconfigure 9 -state disabled
 	    $menu entryconfigure 10 -state disabled
 	    $menu entryconfigure 11 -state disabled
+	    $menu entryconfigure 12 -state disabled
 	}
     } else {
 	set menu $fakerowmenu
@@ -9102,6 +9103,68 @@ proc cherrypick {} {
     notbusy cherrypick
 }
 
+proc rebase_ok {id} {
+    if {[exec git merge-base $id HEAD] ne $id} {
+	error_popup [mc "You cannot edit commits outside of the current branch."]
+	return 0
+    }
+    set headref [exec git symbolic-ref HEAD]
+    set allrefs [split [exec git for-each-ref --format=%(refname)] "\n"]
+    set otherrefs {}
+    set otherrefsneg ""
+    foreach ref $allrefs {
+	if {$ref eq "refs/stash" || $ref eq $headref} continue
+	set otherrefsneg "$otherrefsneg^$ref\n"
+    }
+
+    # Check whether some other refs contain the commit to be edited
+    if {[exec git rev-list --stdin $id << $otherrefsneg] eq {}} {
+	if { [confirm_popup [mc "The commit you are going to edit is contained in another, possibly non-local, ref (e.g. branch or tag).\
+				It is a bad idea to change a ref 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 {![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 --no-autosquash $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 discard them (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---