git rebase interactive: usability issue

Hello All,

Today I got another user complaining that git rebase interactive
sometimes squashing changes without being told to do that. Studying the
reflog revealed what I expected to see: the user started the process of
editing of  chain of patches started by "git rebase -i", and then used
"git commit --amend" to correct some of them, but at some point the
process of rebasing was stopped due to a conflict caused by some previous
changes. The user after resolving this conflict run "git commit --amend"
as he did before, without realising that this time it will squash the
current patch with the previous one.

Though the user realized his mistake after my explanation of how git
rebase works, I still believe it is a serious usability issue, because
the same command: "git commit --amend" produces drastically different
results depends on whether the rebase process stopped due to conflict
or on the "edit" mark. Moreover, the commit message of second patch is
getting lost as result of using "git commit --amend" in the former case.

Personally, I have avoided this issue because normally I don't use git
commit --amend during rebasing unless I have to correct the commit
message. Instead, I just edit files and then do "git add" on them and
then run "git rebase --continue". But latest versions of git suggest
you use "git commit --amend" during interactive rebasing and following
this advice, it is easy to fall into this trap.

The following patch disables "git commit --amend" during rebase when
the process was stopped due to conflict.

-- >8 --
From: Dmitry Potapov <redacted>
Date: Wed, 25 Jun 2008 23:23:22 +0400
Subject: [PATCH] don't allow 'commit --amend' during rebase conflict resolution

Running 'commit --amend' during git rebase is almost certainly a mistake,
which causes that two consequent patches are squashed together. Moreover,
the commit message of the second commit is silently lost. It is almost
certainly not what the user expects. In that very unlikely case when you
really want to combine two patches during rebase conflict resolution,
you can do that using "git reset --soft HEAD^" followed by "git commit".

Signed-off-by: Dmitry Potapov <redacted>
---
 builtin-commit.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e3ad38b..d03696f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c

@@ -925,6 +925,17 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage);
 
+	strbuf_init(&sb, 0);
+	if (amend)
+	{
+		strbuf_addf(&sb, "%s/.dotest-merge", get_git_dir());
+		if (!access(sb.buf, F_OK)) {
+			strbuf_addstr(&sb,"/amend");
+			if (access(sb.buf, F_OK))
+				die("amend not committed yet patch?");
+		}
+	}
+
 	index_file = prepare_index(argc, argv, prefix);
 
 	/* Set up everything for writing the commit object.  This includes

@@ -937,7 +948,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	/*
 	 * The commit object
 	 */
-	strbuf_init(&sb, 0);
+	strbuf_reset(&sb);
 	strbuf_addf(&sb, "tree %s\n",
 		    sha1_to_hex(active_cache_tree->sha1));
 

-- 
1.5.6

Hi,

On Thu, 26 Jun 2008, Dmitry Potapov wrote:

-- >8 --
From: Dmitry Potapov <redacted>
Date: Wed, 25 Jun 2008 23:23:22 +0400
Subject: [PATCH] don't allow 'commit --amend' during rebase conflict resolution

This is a very funny way to interpret the SubmittingPatches document.  
Just stating a fact here.

Running 'commit --amend' during git rebase is almost certainly a 
mistake, which causes that two consequent patches are squashed together. 
Moreover, the commit message of the second commit is silently lost. It 
is almost certainly not what the user expects. In that very unlikely 
case when you really want to combine two patches during rebase conflict 
resolution, you can do that using "git reset --soft HEAD^" followed by 
"git commit".

NACK.

You just broke the 'edit' command.

Ciao,
Dscho

On Thu, Jun 26, 2008 at 03:32:08AM +0400, Dmitry Potapov wrote:

Though the user realized his mistake after my explanation of how git
rebase works, I still believe it is a serious usability issue, because
the same command: "git commit --amend" produces drastically different
results depends on whether the rebase process stopped due to conflict
or on the "edit" mark. Moreover, the commit message of second patch is
getting lost as result of using "git commit --amend" in the former case.

This is true whether it a conflict is getting addressed during a
git-rebase or a git-merge.  The observation I would make is that git
has stopped a rebase or a merge with a conflict that the user needs to
fix up, a "git commit --amend" is almost always the wrong thing.  So I
could imagine a safety where after git discovers a merge conflict, it
sets a flag (probably a file in the .git directory) which causes "git
commit --amend" stop withan error message "this probably wasn't what
you wanted", and telling the user to use a --force command if this is
what they wanted.  This flag would be cleared on the next "git commit"
or "git reset".

In fact, we do this already for git-merge.  Why not just do the same
thing in the middle of a merge conflict with git-rebase?

      	     	       	       		- Ted

Running 'commit --amend' during git rebase is almost certainly a mistake,
which causes that two consequent patches are squashed together. Moreover,
the commit message of the second commit is silently lost. It is almost
certainly not what the user expects. In that very unlikely case when you
really want to combine two patches during rebase conflict resolution,
you can do that using "git reset --soft HEAD^" followed by "git commit".
---

On Thu, Jun 26, 2008 at 08:13:03AM -0400, Theodore Tso wrote:

In fact, we do this already for git-merge.  Why not just do the same
thing in the middle of a merge conflict with git-rebase?

Thank you for suggestion. I have corrected my patch to so the same as
we do in the case of git-merge conflict. MERGE_MSG is already removed
on successful commit, so the patch is very simple now.

 builtin-commit.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e3ad38b..6d1d955 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c

@@ -725,6 +725,10 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die("You have nothing to amend.");
 	if (amend && in_merge)
 		die("You are in the middle of a merge -- cannot amend.");
+	/* no MERGE_HEAD but MERGE_MSG means a conflict during rebase */
+	if (amend && !access(git_path("MERGE_MSG"), F_OK))
+		die("You are in the middle of a rebase conflict -- "
+			"cannot amend.");
 
 	if (use_message)
 		f++;

-- 
1.5.6.60.gbc566