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

[Elijah Newren <newren@gmail.com>]

On Wed, Mar 11, 2020 at 9:34 AM Jeff King <peff@peff.net> wrote:
>
> 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).

> > +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.