Re: [PATCH v2 1/2] sequencer: clear state upon dropping a become-empty commit

From: Jeff King <peff@peff.net>
Date: Mar 11, 2020, 16:34 UTC
Message-ID: <20200311163454.GC27893@coredump.intra.peff.net>
In-Reply-To: <84b89d7843527d7b1f3e00f122dc1e82f1ced381.1583940623.git.gitgitgadget@gmail.com>

On Wed, Mar 11, 2020 at 03:30:22PM +0000, Elijah Newren via GitGitGadget wrote:

Show 13 quoted lines

> From: Elijah Newren <newren@gmail.com>
> 
> In commit e98c4269c8 ("rebase (interactive-backend): fix handling of
> commits that become empty", 2020-02-15), the merge backend was changed
> to drop commits that did not start empty but became so after being
> applied (because their changes were a subset of what was already
> upstream).  This new code path did not need to go through the process of
> creating a commit, since we were dropping the commit instead.
> Unfortunately, this also means we bypassed the clearing of the
> CHERRY_PICK_HEAD and MERGE_MSG files, which if there were no further
> commits to cherry-pick would mean that the rebase would end but assume
> there was still an operation in progress.  Ensure that we clear such
> state files when we decide to drop the commit.

Thanks, I can confirm this fixes my case (which is not surprising, as it is the same as your new test). The patch looks good. Two minor comments below, but I doubt there is anything to act on.

Show 13 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index 7477b15422a..e528225e787 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1957,6 +1957,8 @@ static int do_pick_commit(struct repository *r,
>  		flags |= ALLOW_EMPTY;
>  	} else if (allow == 2) {
>  		drop_commit = 1;
> +		unlink(git_path_cherry_pick_head(r));
> +		unlink(git_path_merge_msg(r));
>  		fprintf(stderr,
>  			_("dropping %s %s -- patch contents already upstream\n"),
>  			oid_to_hex(&commit->object.oid), msg.subject);

It feels like the set of paths to be cleaned up would probably exist elsewhere in a helper function for cleaning up real cherry-picks. But I'll defer to your expertise there, as I don't know the sequencer code very well.

Show 7 quoted lines

> +test_expect_success 'rebase --merge does not leave state laying around' '
> +	git checkout -B testing localmods~2 &&
> +	git rebase --merge upstream &&
> +
> +	test_path_is_missing .git/CHERRY_PICK_HEAD &&
> +	test_path_is_missing .git/MERGE_MSG
> +'

This could check the output of git-status to avoid poking around in the .git directory itself. But I doubt that the exact filenames are going to change, and parsing the output of status is its own problem (I don't think we give this "state" info in a machine-readable way).

-Peff