git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] rebase -i: be careful to wrap up fixup/squash chains

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 4, 2018, 16:48 UTC
Message-ID
<xmqqftypb44v.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<0c9d0f75fc0dd28d55d4ed41d008182746fc86cd.1535759099.git.gitgitgadget@gmail.com>
Show 23 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 84bf598c3e..ac5c805c14 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3578,9 +3578,20 @@ static int commit_staged_changes(struct replay_opts *opts,
>  		 * the commit message and if there was a squash, let the user
>  		 * edit it.
>  		 */
> -		if (is_clean && !oidcmp(&head, &to_amend) &&
> -		    opts->current_fixup_count > 0 &&
> -		    file_exists(rebase_path_stopped_sha())) {
> +		if (!is_clean || !opts->current_fixup_count)
> +			; /* this is not the final fixup */
> +		else if (oidcmp(&head, &to_amend) ||
> +			 !file_exists(rebase_path_stopped_sha())) {
> +			/* was a final fixup or squash done manually? */
> +			if (!is_fixup(peek_command(todo_list, 0))) {
> +				unlink(rebase_path_fixup_msg());
> +				unlink(rebase_path_squash_msg());
> +				unlink(rebase_path_current_fixups());
> +				strbuf_reset(&opts->current_fixups);
> +				opts->current_fixup_count = 0;
> +			}

Let me see if the code is easily grokkable by (trying to) follow aloud.

    We used to refrain from going into this big else clause that
    does the fixup-squash handling when is_clean is false,
    current-count is not yet zero, head and to-amend are different
    commits or stopped-sha file is missing.  The updated code still
    refrains from going into the big else clause under exactly the
    same condition, but it learned to clean up the state, when the
    _next_ one is not a fix-up, i.e. when we are looking at the last
    fixup/squash in the current chain.  And the lack of clean-up
    would have resulted in the next step misbehaving.

I see a few calls to is_fixup(peek_command()) and a local boolean variable final_fixup used in this function. I have to wonder if it makes the resulting code, especially the above part, easier to follow and understand, if the function peeked todo-list to check if we are dealing with the final fix-up in a chain very early just once, and used it to see "are we doing the final fixup/squash in the current chain?" throughout the rest of the function.

	Side note: I actually think that the existing final_fixup
	boolean means something different (iow, final_fixup is not
	set inside the new "clean-up" code above, even though we
	dealt with the last one in the fix-up chain, and that is not
	a bug---which means that "final_fixup" does not mean "we are
	dealing with the last one in the fix-up chain"), which may
	want to be clarified a bit with in-code comment near where
	the variable is defined for the function to be readable.

In any case, thanks for fixing this, which seems to have appeared in Git 2.18. Let's fork a topic from maint, cook it in 'next' and aim for eventually merging it down for both 2.19 and 2.18 tracks.

Show 17 quoted lines
> +		} else {
> +			/* we are in a fixup/squash chain */
>  			const char *p = opts->current_fixups.buf;
>  			int len = opts->current_fixups.len;
>  
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 7d5ea340b3..13f5688135 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -330,7 +330,7 @@ test_expect_success 'wrapped original subject' '
>  	test $base = $parent
>  '
>  
> -test_expect_failure 'abort last squash' '
> +test_expect_success 'abort last squash' '
>  	test_when_finished "test_might_fail git rebase --abort" &&
>  	test_when_finished "git checkout master" &&
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin
Message 7 of 8 in “rebase --autosquash: handle manual "final fixups"”
  1. 0/2 rebase --autosquash: handle manual "final fixups"Johannes Schindelin via GitGitGadget, Aug 31, 2018
  2. 1/2 rebase -i --autosquash: demonstrate a problem skipping the last squashJohannes Schindelin via GitGitGadget, Aug 31, 2018
  3. Re*: [PATCH 1/2] rebase -i --autosquash: demonstrate a problem skipping the last squashJunio C Hamano, Sep 4, 2018
  4. Johannes SchindelinSep 4, 2018
  5. Junio C HamanoSep 5, 2018
  6. 2/2 rebase -i: be careful to wrap up fixup/squash chainsJohannes Schindelin via GitGitGadget, Aug 31, 2018
  7. Junio C HamanoSep 4, 2018
  8. Johannes SchindelinSep 4, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.