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 <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68