From: Mark Jamsek Subject: Re: got backout/cherrypick metadata (for commit log messages) To: Game of Trees Date: Thu, 26 Jan 2023 20:34:23 +1100 On 23-01-26 09:50AM, Stefan Sperling wrote: > On Thu, Jan 26, 2023 at 06:04:43PM +1100, Mark Jamsek wrote: > > As a quick recap, when the user runs 'got {backout,cherrypick} commit', > > we create a "logmsg" reference pointing to the tree's base commit. What > > it points at is largely irrelevant for now, but the reference path takes > > the form: > > > > refs/got/worktree/cherrypick-${WORKTREE_UUID}-$cherry-picked-commit-id} > > refs/got/worktree/backout-${WORKTREE_UUID}-${backed-out-commit-id} > > > > As such, when the user next runs 'got commit', we parse the log > > message(s) from the ${backed-out,cherry-picked}-commit-id refs with > > which we prepopulate the editor. This way, if the user invokes N 'got > > {bo,cy}' commands, all N log messages will appear in the editor. This is > > the primary reason I think this idea of stsp's is better than the first. > > The other being what op observed: no change is required from the user; > > just cherrypick and commit! > > Thanks for taking the time to work on this! No worries! Honestly, you did the hard part by designing the algorithm, implementing it was a simple process after that. > Yes, I think we should try this. If it becomes a burden somehow we can > keep refining the feature, or even rip it out again in a later release > and implement the -a option approach instead. > > I think it would be nice to have this feature in-tree, plus some tests. I'll get some tests written then resubmit along with your doc improvements for review. > > Second, when 'got commit' is run, we loop through each logmsg ref and if > > at least one changed path in the referenced bo/cy commit has changes in > > the work tree that are now being committed, we append its log message to > > the temp file opened in the editor. > > > > Upon a successful commit, we remove all references created by previous > > backout/cherrypick operations in the current work tree. If a commit is > > aborted or otherwise fails, the refs are kept on disk. > > Reading this, I am wondering if we should tweak the behaviour regarding > removal after commit: Would it be better to keep a ref around if there > is no overlap between the cherrypicked commit's list of changed paths > and the newly created commit's list of changed paths? Yes, I agree, and we already do this! Great minds think alike :) I described that part of the implementation wrong by missing a word: (s/references/matched references). In other words, upon successful commit, we only remove all references that were used to populate the commit's log message. So yes--given our heuristic that determines which log messages are used--any refs whose set of paths complements the new commit's changeset remain on disk. See the ref list loop in lookup_logmsg_ref() where only the refs used in the new commit are added to the queue of refs to be deleted. > > This is not foolproof, there are cases where we will inevitably have > > messages that aren't quite relevant, but as stsp remarked, it's not > > a big deal and the benefit outweights the cost of having to tidy things > > up in the editor on the rare occasion this happens. > > Yes, and we will see how this works out in general usage. > And our documentation should be clear about this limitation. > > Speaking of which: > > > +.It Fl l > > +Display a list of references created by previous cherrypick operations > > +If a > > +.Ar commit > > I think the manual page should talk about this at a higher level > of abstraction. > > Users won't know/care whether cherrypick commands created any references. > What matters to users is that Got will try to remember log messages of > cherrypicked commits and append any relevant log messages to the commit > message template. > > We could say something like: > [[[ > .It Fl l > Display a list of commit log messages recorded by cherrypick operations, > represented by references in the "refs/got/worktree/" reference namespace. > ]]] > > Similar to how documentation of -X options for rebase/histedit just talks > about "backups" rather than references, and how the corresponding -l option > mentions how this is implemented, but otherwise keeps the high-level concepts > in focus. Good idea! I'll make this change in the next revision. Thanks for the feedback and also for making this change very straightforward :) -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68