[PATCH v2 0/4] rebase -i: avoid stale "# This is a combination of" in commit messages

Eric Sunshine pointed out that I had such a commit message in https://public-inbox.org/git/CAPig+cRrS0_nYJJY=O6cboV630sNQHPV5QGrQdD8MW-sYzNFGQ@mail.gmail.com/ and I went on a hunt to figure out how the heck this happened.

Turns out that if there is a fixup/squash chain where the *last* command fails with merge conflicts, and we either --skip ahead or resolve the conflict to a clean tree and then --continue, our code does not do a final cleanup.

Contrary to my initial gut feeling, this bug was not introduced by my rewrite in C of the core parts of rebase -i, but it looks to me as if that bug was with us for a very long time (at least the --skip part).

The developer (read: user of rebase -i) in me says that we would want to fast-track this, but the author of rebase -i in me says that we should be cautious and cook this in `next` for a while.

Fixes since v1:

- Using test_i18ngrep instead of grep, because "This is a combination of
  <N> commits" is marked for translation.

- Added a patch to actually fix `rebase -i` when building with
  GETTEXT_POISON, because we used to assume that numbers are encoded as
  ASCII so that we can increment it when writing the next commit message
  in the fixup/squash chain. This also seems to be a long-standing bug
  that has been with us since the the beginning of the localization of
  rebase -i's commit messages.

- The test case now starts with test_when_finished "test_might_fail git rebase
  --abort" to be allow for failing more gently.

- Fixed grammar of 2/3 (now 3/4): thanks, Eric!

- Fixed the description of the new test case (it purported to test --continue,
  but it really tests --skip).

Johannes Schindelin (4):
  rebase -i: demonstrate bugs with fixup!/squash! commit messages
  rebase -i: Handle "combination of <n> commits" with GETTEXT_POISON
  sequencer: leave a tell-tale when a fixup/squash failed
  rebase --skip: clean up commit message after a failed fixup/squash

 sequencer.c                | 93 ++++++++++++++++++++++++++++----------
 t/t3418-rebase-continue.sh | 22 +++++++++
 2 files changed, 92 insertions(+), 23 deletions(-)

base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293
Published-As: https://github.com/dscho/git/releases/tag/clean-msg-after-fixup-continue-v2
Fetch-It-Via: git fetch https://github.com/dscho/git clean-msg-after-fixup-continue-v2

Interdiff vs v1:
 diff --git a/sequencer.c b/sequencer.c
 index f067b7b24c5..881503a6463 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -1350,19 +1350,18 @@ static int update_squash_messages(enum todo_command command,
  		eol = strchrnul(buf.buf, '\n');
  		if (buf.buf[0] != comment_line_char ||
  		    (p += strcspn(p, "0123456789\n")) == eol)
 -			return error(_("unexpected 1st line of squash message:"
 -				       "\n\n\t%.*s"),
 -				     (int)(eol - buf.buf), buf.buf);
 -		count = strtol(p, NULL, 10);
 -
 -		if (count < 1)
 -			return error(_("invalid 1st line of squash message:\n"
 -				       "\n\t%.*s"),
 -				     (int)(eol - buf.buf), buf.buf);
 +			count = -1;
 +		else
 +			count = strtol(p, NULL, 10);
  
  		strbuf_addf(&header, "%c ", comment_line_char);
 -		strbuf_addf(&header,
 -			    _("This is a combination of %d commits."), ++count);
 +		if (count < 1)
 +			strbuf_addf(&header, _("This is a combination of "
 +					       "several commits."));
 +		else
 +			strbuf_addf(&header,
 +				    _("This is a combination of %d commits."),
 +				    ++count);
  		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
  		strbuf_release(&header);
  	} else {
 @@ -1405,13 +1404,22 @@ static int update_squash_messages(enum todo_command command,
  	if (command == TODO_SQUASH) {
  		unlink(rebase_path_fixup_msg());
  		strbuf_addf(&buf, "\n%c ", comment_line_char);
 -		strbuf_addf(&buf, _("This is the commit message #%d:"), count);
 +		if (count < 2)
 +			strbuf_addf(&buf, _("This is the next commit "
 +					    "message:"));
 +		else
 +			strbuf_addf(&buf, _("This is the commit message #%d:"),
 +				    count);
  		strbuf_addstr(&buf, "\n\n");
  		strbuf_addstr(&buf, body);
  	} else if (command == TODO_FIXUP) {
  		strbuf_addf(&buf, "\n%c ", comment_line_char);
 -		strbuf_addf(&buf, _("The commit message #%d will be skipped:"),
 -			    count);
 +		if (count < 2)
 +			strbuf_addf(&buf, _("The next commit message will be "
 +					    "skipped:"));
 +		else
 +			strbuf_addf(&buf, _("The commit message #%d will be "
 +					    "skipped:"), count);
  		strbuf_addstr(&buf, "\n\n");
  		strbuf_add_commented_lines(&buf, body, strlen(body));
  	} else
 diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
 index 4880bff82ff..693f92409ec 100755
 --- a/t/t3418-rebase-continue.sh
 +++ b/t/t3418-rebase-continue.sh
 @@ -88,7 +88,8 @@ test_expect_success 'rebase passes merge strategy options correctly' '
  	git rebase --continue
  '
  
 -test_expect_success '--continue after failed fixup cleans commit message' '
 +test_expect_success '--skip after failed fixup cleans commit message' '
 +	test_when_finished "test_might_fail git rebase --abort" &&
  	git checkout -b with-conflicting-fixup &&
  	test_commit wants-fixup &&
  	test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
 @@ -99,14 +100,14 @@ test_expect_success '--continue after failed fixup cleans commit message' '
  
  	: now there is a conflict, and comments in the commit message &&
  	git show HEAD >out &&
 -	grep "This is a combination of" out &&
 +	test_i18ngrep "This is a combination of" out &&
  
  	: skip and continue &&
  	git rebase --skip &&
  
  	: now the comments in the commit message should have been cleaned up &&
  	git show HEAD >out &&
 -	! grep "This is a combination of" out
 +	test_i18ngrep ! "This is a combination of" out
  '
  
  test_expect_success 'setup rerere database' '