[PATCH RFC] gitk: Allow commit editing

[PATCH RFC] gitk: Allow commit editing

[Michal Sojka]

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 <sojka@os.inf.tu-dresden.de>
---
 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

Re: [PATCH RFC] gitk: Allow commit editing

[Jeff King]

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

Re: [PATCH RFC] gitk: Allow commit editing

[Chris Packham]

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

Re: [PATCH RFC] gitk: Allow commit editing

[Michal Sojka]

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 <sojka@os.inf.tu-dresden.de>
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 <sojka@os.inf.tu-dresden.de>
---
 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---

Re: [PATCH RFC] gitk: Allow commit editing

[Michal Sojka]

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 <sojka@os.inf.tu-dresden.de>
Date: Fri, 19 Aug 2011 16:33:55 +0200
Subject: [PATCH 2/2] gitk: Learn how to fixup commits


Signed-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>
---
 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---

Re: [PATCH RFC] gitk: Allow commit editing

[Michal Sojka]

On Thu, 18 Aug 2011, 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.
> > 
> > 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.

I added a warning if the edited commit is contained in a remote branch.
Would you consider this sufficient?

>   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.

I added "-p" to rebase. I do not know about the robustness of "-p" as
well, but is anything goes wrong during the rebase, git rebase --abort
hopefully reverts everything.

> 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.

See the next version of the patch in a follow-up mail.

-Michal

[PATCH] gitk: Allow commit editing

[Michal Sojka]

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 if 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 chaning a single letter.

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.

The user is warned if the commit that is going to be edited is
contained in a remote branch.

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

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..83b3307 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,36 @@ 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 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 }
+
+    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]
+    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

Re: [PATCH] gitk: Allow commit editing

[Jeff King]

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.

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"?

-Peff

Re: [PATCH v3] gitk: Allow commit editing

[Michal Sojka]

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---

Re: [PATCH v3] gitk: Allow commit editing

[Jeff King]

On Thu, Aug 25, 2011 at 03:15:42PM +0200, Michal Sojka wrote:

> 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.

Yeah, that makes sense.  Technically you are also rewriting every commit
_after_ the commit in question, so you want to be sure that those aren't
included elsewhere, too. But by definition, any ref which includes them
must also include the commit in question, so I think your test is
sufficient.

> 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.

Yes, that makes sense to me, too.

> > 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.

I was going to suggest that we could actually do the rebase on "bar",
but that is probably too much complexity hiding behind the user's back
(not to mention that there are lots of corner cases where figuring out
the right branch is tough, like "gitk <sha1>").

> The other changes in this version are:
> - added --no-autosquash to rebase invocation

Yeah, that is probably a good idea.

> +    # 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

Minor micro-optimization: this can be "git rev-list -1". You only care
if it produces the one commit, so that's sufficient. Without "-1", git
will keep reporting commits all the way down to the merge base with some
other ref.


Another question I had while thinking about whether or not this whole
idea is sane: what happens with conflicts in later commits that are
caused by your amended changes? Rebase will complain to the terminal,
no? Which the user may or may not even see, depending on how they
started gitk.

-Peff

[PATCH v4] gitk: Allow commit editing

[Michal Sojka]

On Thu, 25 Aug 2011, Jeff King wrote:
> On Thu, Aug 25, 2011 at 03:15:42PM +0200, Michal Sojka wrote:
> > +    # 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
> 
> Minor micro-optimization: this can be "git rev-list -1". You only care
> if it produces the one commit, so that's sufficient. Without "-1", git
> will keep reporting commits all the way down to the merge base with some
> other ref.

Here is a new version with the micro-optimization.

Another minor change is that this patch now applies to gitk repo
(http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git
repo.

-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 |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/gitk b/gitk
index a701e0d..1ca2d00 100755
--- a/gitk
+++ b/gitk
@@ -2481,6 +2481,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}
@@ -8445,18 +8446,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
@@ -9120,6 +9121,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 --max-count=1 << $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---

Re: [PATCH v4] gitk: Allow commit editing

[Jeff King]

On Sat, Aug 27, 2011 at 02:31:02PM +0200, Michal Sojka wrote:

> Here is a new version with the micro-optimization.
> 
> Another minor change is that this patch now applies to gitk repo
> (http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git
> repo.

Thanks. This version fixes most of my complaints and looks reasonable
sane, as long as you accept the idea that starting an interactive rebase
behind the scenes is sane.  I'm still not 100% convinced. But then, I am
not a big gitk user, either (I like visualizing with it, but I rarely
want to do anything interactive). So I'll leave Paul and anyone else to
argue or make a decision on that count.

-Peff

Re: [PATCH v4] gitk: Allow commit editing

[Paul Mackerras]

On Sat, Aug 27, 2011 at 02:31:02PM +0200, Michal Sojka wrote:
> Here is a new version with the micro-optimization.
> 
> Another minor change is that this patch now applies to gitk repo
> (http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git
> repo.
> 
> -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.

I have to say that this patch makes me pretty nervous.  I can see the
attractiveness of the feature, but I don't like making gitk
unresponsive for a potentially long time, i.e. until git gui exits.
It may not be clear to users that the reason gitk isn't responding is
because some other git gui window is still running.

Also, if some subsequent commit no longer applies because of the
changes you make to a commit, it's going to abort the rebase
completely and thus lose the changes you made.  That could be
annoying.

I usually do this by starting a new branch just before the commit I
want to change and then use a combination of the cherry-pick menu item
and git commit --amend.  Maybe something to make that simpler for
users would be good, i.e. automate it a bit but still have it be a
step-by-step process if necessary.  Part of the problem of course is
that neither gitk nor git gui are really designed to be an editing
environment.  In fact you really want an edit/compile/test environment
so you don't introduce new bugs.

Paul.

Re: [PATCH v4] gitk: Allow commit editing

[Michal Sojka]

On Thu, 08 Sep 2011, Paul Mackerras wrote:
> On Sat, Aug 27, 2011 at 02:31:02PM +0200, Michal Sojka wrote:
> > Here is a new version with the micro-optimization.
> > 
> > Another minor change is that this patch now applies to gitk repo
> > (http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git
> > repo.
> > 
> > -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.
> 
> I have to say that this patch makes me pretty nervous.  I can see the
> attractiveness of the feature, but I don't like making gitk
> unresponsive for a potentially long time, i.e. until git gui exits.
> It may not be clear to users that the reason gitk isn't responding is
> because some other git gui window is still running.

I understand this. See below for a possible solution.
 
> Also, if some subsequent commit no longer applies because of the
> changes you make to a commit, it's going to abort the rebase
> completely and thus lose the changes you made.  That could be
> annoying.

Agreed. A solution could be to create a ref called for example
refs/gitk/failed-rebase and then abort the rebase together with
displaying the error message explaining what happened. The edited commit
would remain visible and a user can manually cherry pick remaining
commits and then reset the original branch to this failed-rebase ref.

> I usually do this by starting a new branch just before the commit I
> want to change and then use a combination of the cherry-pick menu item
> and git commit --amend.  Maybe something to make that simpler for
> users would be good, i.e. automate it a bit but still have it be a
> step-by-step process if necessary.  Part of the problem of course is
> that neither gitk nor git gui are really designed to be an editing
> environment.

So what about the following:

1) When user selects "Edit commit", git rebase -i is called, git citool
   is started on background and things will be set up (I do not know yet
   how exactly) so that same callback is called when git citool exits.

2) gitk updates the list of commits and marks the current (detached)
   head by a tag (colored box) saying that there is rebase in progress.
   The same tag would appear if gitk is executed manually during
   interactive rebase.

3) When git citool exits (or when you explicitly select something from
   rebase tag's context menu), rebase would continue.

> In fact you really want an edit/compile/test environment so you don't
> introduce new bugs.

Another possibility would be to allow only editing of the commit
message. Then you don't need compile/test steps and the rebase should
not fail due to conflicts.

I do not think I would like this limitation, but it would certainly be
better then nothing.

-Michal

Re: [PATCH RFC] gitk: Allow commit editing

[Jeff King]

On Fri, Aug 19, 2011 at 02:23:39PM +0200, Michal Sojka wrote:

> > 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.
> 
> I added a warning if the edited commit is contained in a remote branch.
> Would you consider this sufficient?

It's likely problematic if the commit appears in the history of any
other ref, unless they are also planning on rebasing that ref, too
(which you can't really know, but it is probably better to warn).

-Peff