# [PATCH] gitk: Allow commit editing

10 messages from 2011-08-19 to 2011-09-13. Participants: Michal Sojka, Jeff King, Paul Mackerras.
Thread: https://gitlist.dev/t/28163

## Michal Sojka, 2011-08-19 12:23

Subject: Re: [PATCH RFC] gitk: Allow commit editing
Message-ID: <87obzlwpx0.fsf@steelpick.2x.cz>
URL: https://gitlist.dev/e/87obzlwpx0.fsf%40steelpick.2x.cz
In-Reply-To: <20110818223346.GA8481@sigill.intra.peff.net>

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

```

## Michal Sojka, 2011-08-19 12:25

Subject: [PATCH] gitk: Allow commit editing
Message-ID: <1313756753-26498-1-git-send-email-sojka@os.inf.tu-dresden.de>
URL: https://gitlist.dev/e/1313756753-26498-1-git-send-email-sojka%40os.inf.tu-dresden.de
In-Reply-To: <87obzlwpx0.fsf@steelpick.2x.cz>

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

```

## Jeff King, 2011-08-25 03:07

Subject: Re: [PATCH RFC] gitk: Allow commit editing
Message-ID: <20110825030755.GA3465@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20110825030755.GA3465%40sigill.intra.peff.net
In-Reply-To: <87obzlwpx0.fsf@steelpick.2x.cz>

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

```

## Jeff King, 2011-08-25 03:14

Subject: Re: [PATCH] gitk: Allow commit editing
Message-ID: <20110825031420.GB3465@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20110825031420.GB3465%40sigill.intra.peff.net
In-Reply-To: <1313756753-26498-1-git-send-email-sojka@os.inf.tu-dresden.de>

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

```

## Michal Sojka, 2011-08-25 13:15

Subject: Re: [PATCH v3] gitk: Allow commit editing
Message-ID: <87bovdvdhd.fsf@steelpick.2x.cz>
URL: https://gitlist.dev/e/87bovdvdhd.fsf%40steelpick.2x.cz
In-Reply-To: <20110825031420.GB3465@sigill.intra.peff.net>

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

```

## Jeff King, 2011-08-25 17:30

Subject: Re: [PATCH v3] gitk: Allow commit editing
Message-ID: <20110825173018.GA519@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20110825173018.GA519%40sigill.intra.peff.net
In-Reply-To: <87bovdvdhd.fsf@steelpick.2x.cz>

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

```

## Michal Sojka, 2011-08-27 12:31

Subject: [PATCH v4] gitk: Allow commit editing
Message-ID: <87fwknaveh.fsf@steelpick.2x.cz>
URL: https://gitlist.dev/e/87fwknaveh.fsf%40steelpick.2x.cz
In-Reply-To: <20110825173018.GA519@sigill.intra.peff.net>

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

```

## Jeff King, 2011-09-07 20:10

Subject: Re: [PATCH v4] gitk: Allow commit editing
Message-ID: <20110907201011.GF13364@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20110907201011.GF13364%40sigill.intra.peff.net
In-Reply-To: <87fwknaveh.fsf@steelpick.2x.cz>

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

```

## Paul Mackerras, 2011-09-08 20:59

Subject: Re: [PATCH v4] gitk: Allow commit editing
Message-ID: <20110908205945.GB8091@bloggs.ozlabs.ibm.com>
URL: https://gitlist.dev/e/20110908205945.GB8091%40bloggs.ozlabs.ibm.com
In-Reply-To: <87fwknaveh.fsf@steelpick.2x.cz>

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

```

## Michal Sojka, 2011-09-13 23:11

Subject: Re: [PATCH v4] gitk: Allow commit editing
Message-ID: <87litsf36r.fsf@steelpick.2x.cz>
URL: https://gitlist.dev/e/87litsf36r.fsf%40steelpick.2x.cz
In-Reply-To: <20110908205945.GB8091@bloggs.ozlabs.ibm.com>

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

```