From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 82AD11F619 for ; Wed, 11 Mar 2020 19:33:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731330AbgCKTdU (ORCPT ); Wed, 11 Mar 2020 15:33:20 -0400 Received: from cloud.peff.net ([104.130.231.41]:36762 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1731322AbgCKTdT (ORCPT ); Wed, 11 Mar 2020 15:33:19 -0400 Received: (qmail 3625 invoked by uid 109); 11 Mar 2020 19:33:18 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Wed, 11 Mar 2020 19:33:18 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 22391 invoked by uid 111); 11 Mar 2020 19:42:40 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 11 Mar 2020 15:42:40 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 11 Mar 2020 15:33:17 -0400 From: Jeff King To: Elijah Newren Cc: Elijah Newren via GitGitGadget , Git Mailing List , Phillip Wood , Junio C Hamano Subject: Re: [PATCH v2 1/2] sequencer: clear state upon dropping a become-empty commit Message-ID: <20200311193317.GC27978@coredump.intra.peff.net> References: <84b89d7843527d7b1f3e00f122dc1e82f1ced381.1583940623.git.gitgitgadget@gmail.com> <20200311163454.GC27893@coredump.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Mar 11, 2020 at 10:16:22AM -0700, Elijah Newren wrote: > > 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. > > 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.