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
View this message in rfc822 format
From: Sean Whitton <spwhitton <at> spwhitton.name> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: dmitry <at> gutov.dev, eliz <at> gnu.org, 79408 <at> debbugs.gnu.org, juri <at> linkov.net Subject: bug#79408: 31.0.50; VC commands for cherry-picking Date: Tue, 09 Sep 2025 12:08:40 +0100
Hello, 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. > 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? 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. - 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. (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.) >> 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. So unfortunately I think we do have to give up the prefix arg to this. >> 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.) >> vc-cherry-apply: >> The advantage of having a backend API function for this is that the >> backend can use its full merging logic. >> A generic vc-default-cherry-apply can be implementated similarly to >> vc-apply-to-other-working-tree. > > Right. I assume that generic implementation would use the 'diff backend > function. And the sole disadvantage of such a generic implementation is > that it would have less smart merging. It would also be slower, probably, but for cherry-applying instead of cherry-picking, that's less important, because you are probably applying changes from fewer revisions. >> vc-undo-apply: >> Like vc-cherry-apply except passes REVERSE non-nil at the appropriate >> point. Similarly there can be a vc-default-reverse-cherry-apply. > > That sounds good, but... > > I wonder if we even need to support vc-cherry-apply and vc-undo-apply? > At least right now. > > Two very different reasons why we might not want those: > > - Those seem primarily useful when you want to actually edit the diff > before committing. But, it seems to me that using diff-mode for such > an operation is already pretty natural, and should be encouraged. > > The way I do those two operations today in hg is: > - Open up vc-print-log starting at some revision > - Find the revision I want to revert or cherry-apply > - Hit d or D to open up the diff > - Use diff-mode to do the editing > > Perhaps that is sufficient? I think you're right. Let's leave those two out for now. > - 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. 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'. 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. 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. 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 ? -- Sean Whitton
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.