On 23-01-19 05:33PM, Stefan Sperling wrote:
> On Fri, Jan 20, 2023 at 01:43:47AM +1100, Mark Jamsek wrote:
> > On 23-01-19 12:23PM, Stefan Sperling wrote:
> > > On Thu, Jan 19, 2023 at 07:05:44PM +1100, Mark Jamsek wrote:
> > > > I was glad to see this on the todo list because I'll use 'got cy -a'
> > > > a lot! I often run log or tog to copypasta the log message before
> > > > running `got cy branch && got ci -m msg` so I was keen to see this
> > > > implemented.
> > > >
> > > > I'm really looking for feedback though as I'm not sure if the annotated
> > > > log message and base-commit bump stsp mentioned is adequate. Thanks!
> > > 
> > > Your code is not checking pre-conditions. This feature should require
> > > a fully up-to-date and clean work tree to reduce the likelyhood of
> > > problems during the merge and avoid unrelated changes getting mixed in.
> > 
> > Sorry, I did read that part you wrote in the todo and honestly wasn't
> > ignoring it! I intended to do this, I should've been more clear in my
> > email. I was looking for feedback on a couple things I was thinking
> > about while writing this but was late for a meeting and wanted to send
> > the email while it was on my mind in the hopes I'd have some feedback
> > I could apply when I returned. I really should've made the purpose of my
> > email clearer and emphasised its work-in-progress state. Sorry, Stefan,
> > I really didn't want to give the impression I'd just ignore your
> > requirements like that!
> 
> No worries. I didn't mean to complain or sound harsh. I was giving
> brief feedback while riding a train :)

Oh I didn't think you were complaining or sounding harsh. Your remarks--
as always--were perfectly valid and constructive observations. After
I read your email, I reread mine and realised how rude it looked as
though I had completely ignored your todo notes! I wanted to express
that wasn't the case.

Your feedback is always very much appreciated and never comes across as
anything but accurate and helpful :)

> > I've just read the follow-up emails and messages on IRC (I was
> > babysitting tonight so am late to the party :) and I'm happy to drop
> > this if we want to go in another direction. I just got a bit excited
> > when I read the todo this afternoon as it covers a use case I do
> > somewhat often, but it's no hassle to keep doing what I've been doing
> > till we come up with something else.
> 
> I am still undecided. It is very useful to see concrete implementations
> of such ideas to see how they work in practice, so it is good that we
> now have a good patch for cherrypick -a.

At first I was favouring the todo approach, but the alternative not only
adds more flexibility for users now but also for future changes!  I've
consequently become undecided too :)

> If you have the energy to attempt my other suggestion,

I'm definitely keen! Time permitting I'll attempt your alternative
suggestion.

> one way this could be implemented is to have cherrypick and backout
> create references such as:
> 
>   refs/got/worktree/cherrypick-${WORKTREE_UUID}-$cherry-picked-commit-id}
>   refs/got/worktree/backout-${WORKTREE_UUID}-${backed-out-commit-id}
> 
> We need the cherrypick/backout commit ID in the reference name because
> it is possible to run multiple cherrypicks and/or backouts in the same
> work tree without running 'got commit' in-between.
> 
> I am not sure if we could do anything useful with the value this reference
> points at. It could be symref pointing at another reference or even store
> some arbitrary target value which encodes some useful information.
> It could point to an object ID, such as the ID of a blob we create to
> store some useful information.
> Maybe we will want to store the work tree's current branch name here
> (as a symref), or the work tree's current base commit seen when the
> cherrypick command was run.
> Maybe we will want to allow commands other than 'got commit' to detect
> the presence of cherry-picked changes and act upon them somehow, using
> this extra info.
> But, for now, I do not see an obvious use case for the reference target,
> so just pick something to point at and see how it goes.
> 
> Creating the refs requires two or three got_ref_ function calls,
> and some macros in got_lib_worktree.h, and some helper functions to
> build the reference names. This is similar to what rebase/histedit/merge
> are already doing so coding this up should not be very difficult.
> And you don't need to move the work-tree onto a temporary branch like
> those other commans do, so that complexity can be ignored.
> 
> At 'got commit' time we would look for such references and load
> the log message of each cherrypicked commit-id into the commit
> message template, with markers such as:
> 
>   # log message of cherry-picked commit $commit_id
>   # log message of backed-out commit $commit_id
> 
> This would look similar to how 'got histedit -f' shows the log
> messages of folded commits.
> 
> As soon as a commit operation succeeds, any such references for the
> current work tree are removed, since they are only needed once.
> Essentially, a successful commit clears all this meta-data to
> make future cherrypicks/backouts start over.
> 
> We don't really have to worry about local changes in the work tree
> getting out of sync with those refs. It can be the user's job to
> adjust the template accordingly at as needed at commit time.

Thanks for all this! You've clearly laid it out, and it makes a lot of
sense.  At the very least I think I should code it up so we can get
a feel for both. Given the new suggestion satisfies the 1:1 and *:1
backout/cherrypick:commit use case, if we have to choose one, I think
the more flexible approach makes the most sense.

> There are some special cases to consider:
> 
> If the reference already exists because someone runs the same
> cherrypick command multiple times for some reason, either leave
> th ref in place or just overwrite it. No big deal.
> 
> If a commit is first cherry-picked and then backed-out again, should
> we then remove the cherrypick-commit reference, and vice versa?
> Not sure. Maybe just keep both of them and let the user deal with
> tidying things up in their editor?
> 
> While the worktree is performing a rebase/histedit/merge operation,
> no cherrypick/backout references should be created even when the user
> runs cherrypick/backout to manipulate the work tree. This keeps things
> simple since we already have other commit templates in such situations.
> 
> Conversely, the rebase/histedit/merge commands should probably error
> out early if such references are present (even though they do already
> error out in the presence of local changes, which will usually be
> present once we have cherrypick/backout references).
> Which imlies we also need a way to remove them on demand without
> creating a commit. A simple 'got ref -d' should work :)
> Alternatively, rebase/histedit/merge could silently remove such refs
> if they are present and the work tree contains no local changes.

If the user has cherrypicked or backed out some commits and then decides
not to commit, a `got revert` is likely; would it be good to clear the
reverted cherrypick and/or backout references upon `got revert`?
Otherwise, if the user fails to run `got {backout,cherrypick} -X`, the
stale log messages at the next `got commit` could be confusing.

> This scheme is quite simplistic and may not always produce an ideal
> log message template. I suspect cases where this extra log message
> content gets in the way will be rare. The usual sequence of events will
> be one or more 'got cherrypick' or 'got backout' commands, followed by
> 'got commit'. In this common case this feature would be convenient.
> If someone does something more complicated then they will be able to
> adjust the log message accordingly or remove the references if needed.
> 
> In any case, this only affects log message templates so there is very
> little chance of doing lasting damage somehow.

While I think -a is somewhat simpler, the new approach is certainly more
versatile. Although I can't remember cherrypicking or backing out
multiple commits to be committed in one, I'm sure others do and it would
be good to provide for this use case. My slight concern is that users
have to remember running `got {backout,cherrypick} -X` to avoid
confusing themselves in the editor if they're presented with log
messages of abandoned `got {backout,cherrypick}` operations. They can
always `dG`--or their editor's delete-to-EOF equivalent keymap--to clear
them out, but if we can make it work without adding more flags to
backout and cherrypick, that would be super! In either case, thanks for
the detailed hints, I'll give it a go :)

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68