Re: [PATCH RFC] gitk: Allow commit editing

On Thu, 18 Aug 2011, Jeff King wrote:

On Wed, Aug 17, 2011 at 09:56:11PM +0200, Michal Sojka wrote:

quoted

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