Re: [PATCH v2 1/2] sequencer: clear state upon dropping a become-empty commit
On Wed, Mar 11, 2020 at 9:34 AM Jeff King <peff@peff.net> wrote:
Show 39 quoted lines
>
> On Wed, Mar 11, 2020 at 03:30:22PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > 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.
>
> > 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.Yeah, I was looking for something like that but instead found the unlink() directives for cleaning up various state files scattered throughout the code. I think sequencer.c is in need of some cleaning up; the slow transition from "do what shell does, now work both with an external shell and some pieces built in, now move slightly more towards being built-in" seems to have left a lot of artifacts around and made it a bit unwieldy.
As another anecdote along these lines, I really wanted to do my demo of an in-memory rebase with the existing builtin/rebase.c and sequencer.c but it was too much effort even for just a demo to rip out the unwanted parts, so I did my in-memory rebase demo in a completely different file (https://github.com/newren/git/blob/git-merge-2020-demo/builtin/fast-rebase.c)
I'm not sure deferring to my expertise with sequencer.c makes sense, since you have about twice as many commits to sequencer.c as me. But I was deferring to Phillip and he commented on my v1 and seemed happy (other than my missing handling of MERGE_MSG).
Show 12 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).
Yeah, it's not clear to me what's best either. When I was testing my changes locally I was checking status output. However, after creating the fix and deciding to add a regression test, I switched to checking for the existence of those files basically for the reasons you mention, despite knowing I'm only testing for certain state files rather than testing that git in general doesn't think it's still in the middle of some operation.
Elijah Newren via GitGitGadget
