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

From: Jeff King <peff@peff.net>
Date: Mar 11, 2020, 19:33 UTC
Message-ID: <20200311193317.GC27978@coredump.intra.peff.net>
In-Reply-To: <CABPp-BFtspDhSherYyhH-Aw=oPLzCiGfbMUiLPB0n1rnzV3k0Q@mail.gmail.com>

On Wed, Mar 11, 2020 at 10:16:22AM -0700, Elijah Newren wrote:

Show 12 quoted lines

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

OK. As long as you looked and didn't find anything obvious that should be used, I'm content to leave it for a later cleanup (I also looked briefly and didn't find anything useful).

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

Heh, all memories of sequencer.c have been wiped from my memory. I thought you might have looked at it more recently because of this rebase backend work, but I guess that didn't involve poking at the sequencer internals much.

Show 12 quoted lines

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

I did just double check that "git status" has no way to produce a machine-readable version of the data. That might be worth addressing in general[1], but I think what you have here is a good test for now.

-Peff

[1] In particular, I think that git-prompt.sh reimplements some of this
    logic, and I would be surprised if there wasn't some weird corner
    case where they differ. The prompt code does try to avoid invoking
    subprocesses for efficiency, but I imagine we're running git-status
    already to get the dirty state.