git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Johannes Schindelin <johannes.schindelin@gmx.de>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash
Date: Sat, 21 Apr 2018 16:42:12 +0100	[thread overview]
Message-ID: <b6512eae-e214-9699-4d69-77117a0daec3@talktalk.net> (raw)
In-Reply-To: <6d9f6ba1e73d2297cef3619a89ce69122438368d.1524226637.git.johannes.schindelin@gmx.de>

On 20/04/18 13:18, Johannes Schindelin wrote:
> 
> During a series of fixup/squash commands, the interactive rebase builds
> up a commit message with comments. This will be presented to the user in
> the editor if at least one of those commands was a `squash`.
> 
> However, if the last of these fixup/squash commands fails with merge
> conflicts, and if the user then decides to skip it (or resolve it to a
> clean worktree and then continue the rebase), the current code fails to
> clean up the commit message.

Thanks for taking the time to track this down and fix it.

> 
> This commit fixes that behavior.
> 
> The diff is best viewed with --color-moved.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   sequencer.c                | 36 ++++++++++++++++++++++++++++--------
>   t/t3418-rebase-continue.sh |  2 +-
>   2 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index a9c3bc26f84..f067b7b24c5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2781,17 +2781,12 @@ static int continue_single_pick(void)
>   
>   static int commit_staged_changes(struct replay_opts *opts)
>   {
> -	unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
> +	unsigned int flags = ALLOW_EMPTY | EDIT_MSG, is_fixup = 0, is_clean;
>   
>   	if (has_unstaged_changes(1))
>   		return error(_("cannot rebase: You have unstaged changes."));
> -	if (!has_uncommitted_changes(0)) {
> -		const char *cherry_pick_head = git_path_cherry_pick_head();
>   
> -		if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> -			return error(_("could not remove CHERRY_PICK_HEAD"));
> -		return 0;
> -	}
> +	is_clean = !has_uncommitted_changes(0);
>   
>   	if (file_exists(rebase_path_amend())) {
>   		struct strbuf rev = STRBUF_INIT;
> @@ -2804,16 +2799,41 @@ static int commit_staged_changes(struct replay_opts *opts)
>   		if (get_oid_hex(rev.buf, &to_amend))
>   			return error(_("invalid contents: '%s'"),
>   				rebase_path_amend());
> -		if (oidcmp(&head, &to_amend))
> +		if (!is_clean && oidcmp(&head, &to_amend))
>   			return error(_("\nYou have uncommitted changes in your "
>   				       "working tree. Please, commit them\n"
>   				       "first and then run 'git rebase "
>   				       "--continue' again."));
> +		if (is_clean && !oidcmp(&head, &to_amend)) {

Looking at pick_commits() it only writes to rebase_path_amend() if there 
are conflicts, not if the command has been rescheduled so this is safe.

> +			strbuf_reset(&rev);
> +			/*
> +			 * Clean tree, but we may need to finalize a
> +			 * fixup/squash chain. A failed fixup/squash leaves the
> +			 * file amend-type in rebase-merge/; It is okay if that
> +			 * file is missing, in which case there is no such
> +			 * chain to finalize.
> +			 */
> +			read_oneliner(&rev, rebase_path_amend_type(), 0);
> +			if (!strcmp("squash", rev.buf))
> +				is_fixup = TODO_SQUASH;
> +			else if (!strcmp("fixup", rev.buf)) {
> +				is_fixup = TODO_FIXUP;
> +				flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;

I was going to say this should probably be (flags & ~(EDIT_MSG | 
VERIFY_MSG)) but for some reason VERIFY_MSG isn't set here - I wonder if 
it should be as I think it's set elsewhere when we edit the message.

> +			}
> +		}
>   
>   		strbuf_release(&rev);
>   		flags |= AMEND_MSG;
>   	}
>   
> +	if (is_clean && !is_fixup) {
> +		const char *cherry_pick_head = git_path_cherry_pick_head();
> +
> +		if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> +			return error(_("could not remove CHERRY_PICK_HEAD"));
> +		return 0;
> +	}
> +
>   	if (run_git_commit(rebase_path_message(), opts, flags))

If a squash command has been skipped, then rebase_path_message() still 
contains the message of the skipped commit. If it passed NULL instead 
then the user would get to edit the previous version of the squash 
message without the skipped commit message in it.

Also I think we only want to re-commit if the skipped squash/fixup was 
preceded by another squash/fixup. If the user skips the first 
squash/fixup in a chain then HEAD has the commit message from the 
original pick so does not need amending. The first patch could perhaps 
avoid writing rebase_path_amend_type() in that case by reading the 
squash message and checking the message count is greater than two.

Best Wishes

Phillip

>   		return error(_("could not commit staged changes."));
>   	unlink(rebase_path_amend());
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index b177baee322..4880bff82ff 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -88,7 +88,7 @@ test_expect_success 'rebase passes merge strategy options correctly' '
>   	git rebase --continue
>   '
>   
> -test_expect_failure '--continue after failed fixup cleans commit message' '
> +test_expect_success '--continue after failed fixup cleans commit message' '
>   	git checkout -b with-conflicting-fixup &&
>   	test_commit wants-fixup &&
>   	test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> 


  reply	other threads:[~2018-04-21 15:42 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20 12:17 [PATCH 0/3] rebase -i: avoid stale "# This is a combination of" in commit messages Johannes Schindelin
2018-04-20 12:17 ` [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! " Johannes Schindelin
2018-04-20 16:40   ` Stefan Beller
2018-04-20 20:04     ` Johannes Schindelin
2018-04-20 20:38       ` Johannes Schindelin
2018-04-20 20:44       ` Stefan Beller
2018-04-20 21:05         ` Johannes Schindelin
2018-04-20 19:29   ` Eric Sunshine
2018-04-20 19:31     ` Eric Sunshine
2018-04-20 20:52       ` Johannes Schindelin
2018-04-20 12:18 ` [PATCH 2/3] sequencer: leave a tell-tale when a fixup/squash failed Johannes Schindelin
2018-04-20 19:34   ` Eric Sunshine
2018-04-20 12:18 ` [PATCH 3/3] rebase --skip: clean up commit message after a failed fixup/squash Johannes Schindelin
2018-04-21 15:42   ` Phillip Wood [this message]
2018-04-27 21:36     ` [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b6512eae-e214-9699-4d69-77117a0daec3@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).