Re: [PATCH RFC] gitk: Allow commit editing
- From
Michal Sojka <sojka@os.inf.tu-dresden.de>
- Date
- Aug 19, 2011, 12:23 UTC
- Message-ID
- <87obzlwpx0.fsf@steelpick.2x.cz>
- In-Reply-To
- <20110818223346.GA8481@sigill.intra.peff.net>
On Thu, 18 Aug 2011, Jeff King wrote:
Show 17 quoted lines
> 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?
Show 5 quoted lines
> 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