Package: emacs;
Reported by: Sean Whitton <spwhitton <at> spwhitton.name>
Date: Mon, 8 Sep 2025 11:28:02 UTC
Severity: normal
Found in version 31.0.50
Message #20 received at 79408 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Sean Whitton <spwhitton <at> spwhitton.name> Cc: dmitry <at> gutov.dev, eliz <at> gnu.org, juri <at> linkov.net, 79408 <at> debbugs.gnu.org Subject: Re: bug#79408: 31.0.50; VC commands for cherry-picking Date: Tue, 09 Sep 2025 11:07:21 -0400
Sean Whitton <spwhitton <at> spwhitton.name> writes: > On Mon 08 Sep 2025 at 02:56pm -04, Spencer Baugh wrote: > >> This reminded me of how we don't have a way to do format-patch using vc; >> that's somewhat related, and would be nice to support. > > We do! 'M-x vc-prepare-patch' is the interactive entry point. Ah, right. I never use that because I always interact with Emacs development via Gnus, mostly via debbugs.el, and I just manually attach patches to my emails. (debbugs.el also has some way to format a patch but I've never gotten it to work) >> In fact, I wonder if perhaps: >> >> - a new format-patch backend function >> >> - a new apply-patch backend function (to take advantage of better >> backend-specific merging) >> >> - maybe a fancier checkin-patch backend function >> >> Could replace the need for these specialized backend functions? >> >> (I think git-cherry-pick used to be implemented as format-patch + apply >> under the hood, years ago) > > So, the generic code would be responsible for invoking format-patch, > then apply-patch, then checkin-patch, in that order, right? No. It would be format-patch followed by either apply-patch or checkin-patch. format-patch would be "git format-patch" apply-patch would be "git apply" (a new version of) checkin-patch would be "git am" > A couple of tricky things: > > - We would need a way to extract authorship information and then pass > that into the improved checkin-patch. An author's name and e-mail > address is generic enough, but the representation of dates might be > something highly backend-specific. It's another couple of API > functions. Actually, I don't think we would need that. The date and author information are both included in headers in the files produced by "git format-patch", and that information is used by "git am". Likewise, "hg export" produces patch files which contain the date and author, and "hg import" uses that information. The exact format of the patch is backend-specific, but I think we could reasonably rely on backends having some way to export a commit in the form of some header followed by a diff, which can then be imported again by another backend function. > - There might be conflicts, in both the sense of ordinary diff > application conflicts, and for Git, issues with the staging area. > (There is a whole pile of logic in vc-git-checkin for dealing with the > staging area with a comment summarising some of the issues, and there > are still broken edge cases, I think.) > I'm thinking that if the underlying VCS is responsible for handling > the whole operation, it will manage to complete the cherry-pick anyway > in more situations than our generic code will be able to do. > It will also be faster, which might matter when cherry-picking a big > chunk of a branch. > > Thinking about these sorts of issues, I am inclined to prefer > cherry-pick-specific API functions. They're not hard for a backend > author to implement, and I don't think they preclude adding an > apply-patch and a fancier checkin-patch too for other purposes. The VCS would still be (basically) handling the whole thing. I'm suggesting that we basically just run: git format-patch | git am or: hg import | hg export Which (I think) is how cherry-picking works under the hood historically in git anyway. > (In particular, your apply-patch is very interesting, I agree that might > be a nice addition for diff-mode. Then 'C-c RET a' would call into the > VCS, sometimes. That's probably a separate bug though.) Right. And if we're leaving out support for cherry-apply-without-commit for now, then we don't need to consider apply-patch right now anyway. >>> These are my proposed commands: >>> >>> vc-cherry-pick: >>> - Gets the log message for REV with get-change-comment API function. >>> - Gets the string to append with cherry-pick-comment API function. >>> - Puts these together, starts a Log Edit session for the user to amend >>> the message (e.g. for Git, doing C-c C-s to add a sign-off). >>> - On C-c C-c, calls the cherry-pick API function for the backend to do >>> the cherry-pick. >>> Examples: vc-git-cherry-pick would invoke 'git cherry-pick' or >>> 'git revert' depending on whether REVERT. >>> vc-hg-cherry-pick would invoke 'hg graft' or 'hg backout'. >>> >>> Will have vc-checkin's COMMENT, INITIAL-CONTENTS arguments so calling >>> from Lisp can skip the Log Edit session. >>> >>> A prefix argument might toggle whether to append the cherry-pick >>> comment. >> >> That sounds like a bit of a waste of the prefix argument, since the user >> could always just delete that comment. I think we'd be better off just >> not having the prefix argument do anything for now, and maybe we'll find >> a better use in the future. > > That works fine when cherry-picking a single commit, but if > cherry-picking multiple commits, we're thinking there'll no opportunity > to edit the commit message. In my experience cherry-picking, I want the > cherry-pick comment often, but definitely not always. > > Git changed their default for this at one point; -x used to be implicit, > now you have to specify it. Ah, right... Maybe we could prompt with Log Edit for each commit, even when cherry-picking multiple commits? I guess that would make cherry-picking multiple commits quite annoying... Oh, what if before opening any Log Edit buffers, we prompt the user whether they want to edit the commit messages for the commits being cherry-picked? We could give them several options: - don't manually edit messages but add cherry-pick message to each of them - don't manually edit messages and don't add cherry-pick message - manually edit all the messages This would give the user the ability to choose to edit commit messages even when cherry-picking multiple commits, which might be nice. Or... an even more radical idea would be to let them pick and choose which commit messages they want to edit, and even allow them to stop and edit individual commits in a series. Similar to git rebase -i. Actually, don't we need to be able to stop in the middle of cherry-picking a series of commits anyway, since we need to be able to resolve conflicts? I think that implies a substantially more rich UI then we've been talking about so far... >>> vc-undo-revision (vc-revert is taken): >>> Same as vc-cherry-pick, except passes REVERSE non-nil at the >>> appropriate points. No prefix argument. >> >> Maybe we could name it vc-revert-revision? >> >> Actually maybe we should name the commands something like: >> >> vc-revision-cherry-pick >> vc-revision-cherry-pick-apply >> vc-revision-revert >> vc-revision-revert-apply >> >> To display more directly how they're closely related and symmetric. > > Yes, those are much better, thank you. > > (I would just say vc-revision-cherry-apply over > vc-revision-cherry-pick-apply, though.) Reasonable. >> - In a completely different direction: what if we only supported >> cherry-pick, but also added support for git reset --soft? In git, I >> often do cherry-apply by just cherry-picking and then resetting. This >> is a bit weird, but it might save us some complexity if we want to >> support git reset --soft anyway? > > There are a number of convenient workflows that I've used, and seen > people discuss, which rely on creative uses of 'git reset --soft'. > So I think it would be a very welcome addition. It also means one could > immediately use these workflows and tricks with other VCS without > learning about what the equivalent operation is, because Emacs would > know. Indeed. > In generic VC terms, we could have an operation that deletes revisions > from the end of the branch, and a version of that operation that deletes > the revisions without removing the changes. The former would be > 'git reset --hard' and the latter would be 'git reset --soft'. Makes sense. > We already have vc-allow-rewriting-published-history which we can use to > protect the user from getting themselves into an inconsistent state by > means of these operations. Hm, I guess you're saying we would allow resetting commits which haven't been pushed, but not (with nil vc-allow-rewriting-published-history) resetting commits which have been pushed? That seems reasonable. > I think we want to bake in the end-of-current-branch part. > Here is what I mean. If we call it "vc-delete-revision", then it > invites the question as to whether you can delete revisions that are not > at the end of the branch. But that's a completely different operation: > it's a rebase, not merely a reset. It's much less VCS-generic how such > a thing could work. So I propose to exclude it. I agree that we should only operate on HEAD/tip, and the name should connote this. > So I think we could call this something like > > - vc-delete-back-to-revision > - vc-undo-checkins > - vc-undo-checkins-back-to > - vc-strip-revisions-back-to > > ? vc-undo-checkins might be confusing because actually this command can also be used on revisions which weren't created locally by vc-checkin. The "back-to" names feel a bit verbose/clumsy to me. What about mentioning tip in the name? For example: - vc-delete-tip-revisions - vc-strip-tip-revisions - vc-reset-tip-revisions Or maybe just vc-reset-revisions on its own would work as a name? "git reset" already only operates on tip, so there's prior art. I guess if we're going with the "vc-revision-cherry-pick" scheme, we might want to match that convention, with e.g. "vc-revision-reset".
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.