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:
> > > As per the subject and todo item, the below diff introduces the -a flag
> > > to both the backout and cherrypick commands.
> > > 
> > > 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 :)

> 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.

If you have the energy to attempt my other 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.

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.


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.