Backport of d7a78f8

- `choice` is not a proper type
- markers should be declared

In some cases, feedback to the PR author that an r+ is redundant went
missing.

This turns out to be due to the convolution of the handling of
approval on forward-port, and the fact that the target PR is treated
exactly like its ancestors: if the PR is already approved the approval
is not even attempted (and so no feedback if it's incorrect).

Straighten up this bit and add a special case for the PR being
commented on, it should have the usual feedback if in error or already
commented on.

Furthermore, update `PullRequests._pr_acl` to kinda work out of the
box for forward-port: if the current PR is a forward port,
`is_reviewer` should check delegation on all ancestors, there doesn't
seem to be any reason to split "source_reviewer", "parent_reviewer",
and "is_reviewer".

Fixes #939

The FW reminder is useful to remind people of the outstanding forward
ports they need to process, as taking too long can be an issue.

They are, however, not useful if the developer has already done
everything, the PR is ready and unblocked, but the mergebot has fallen
behind and has a hard time catching up. In that case there is nothing
for the developer to do, so pinging them is not productive, it's only
frustrating.

Fixes #930

Reminding users that `r-` on a forward port only unreviews *that*
forwardport is useful, as `r+;r-` is not a no-op (all preceding
siblings are still reviewed).

However it is useless if all siblings are not approved or already
merged. So avoid sending the warning in that case.

Fixes #934

Rather than only setting `staging_end` on status change, set it when
the staging gets deactivated. This way cancelling a staging (whether
explicitely or via a PR update) will also end it, so will a staging
timeout, etc..., rather than keep the counter running.

Fixes #931

This is an approximation under the assumption that stored computes
update the `write_date`, and that there's not much else that will be
computed on a batch.

Eventually it might be a good idea for this to be a proper field,
computed alongside the unblocking of the batch.

Fixes #932

Today (or really a month ago) I learned: when giving git a symbolic
ref (e.g. a ref name), if it's ambiguous then

1. If `$GIT_DIR/$name` exists, that is what you mean (this is usually
   useful only for `HEAD`, `FETCH_HEAD`, `ORIG_HEAD`, `MERGE_HEAD`,
   `REBASE_HEAD`, `REVERT_HEAD`, `CHERRY_PICK_HEAD`, `BISECT_HEAD` and
   `AUTO_MERGE`)
2. otherwise, `refs/$name` if it exists
3. otherwise, `refs/tags/$name` if it exists
4. otherwise, `refs/heads/$name` if it exists
5. otherwise, `refs/remotes/$name` if it exists
6. otherwise, `refs/remotes/$name/HEAD` if it exists

This means if a tag and a branch have the same name and only the name
is provided (not the full ref), git will select the tag, which gets
very confusing for the mergebot as it now tries to rebase onto the tag
(which because that's not fun otherwise was not even on the branch of
the same name).

Fix by providing full refs to `rev-parse` when trying to retrieve the
head of the target branches. And as a defense in depth opportunity,
also exclude tags when fetching refs by spec: apparently fetching a
specific commit does not trigger the retrieval of tags, but any sort
of spec will see the tags come along for the ride even if the tags are
not in any way under the fetched ref e.g. `refs/heads/*` will very
much retrieve the tags despite them being located at `refs/tags/*`.

Fixes #922

Apparently when I added these to trigger the corresponding cron the
lack of decorator caused them to not work at all, at least when
invoked from the interface, consequently I notably wasn't able to
force a forward port via creating one such task when trying to work
around #953

Logging PRs by id is unusual, unreadable, and inconvenient.

- rather than enumerate states, forward-porting should just check if
  the statuses are successful on a PR
- for the same consistency reasons explained in
  f97502e, `skipchecks` should force
  the status of a PR to `success`: it very odd that a PR would be
  ready without being validated...

The UX around the split of limit and forward port policy (and
especially moving "don't forward port" to the policy) was not really
considered and caused confusion for both me and devs: after having
disabled forward porting, updating the limit would not restore it, but
there would be no indication of such an issue.

This caused odoo/enterprise#68916 to not be forward ported at merge
(despite looking for all the world like it should be), and while
updating the limit post-merge did force a forward-port that
inconsistency was just as jarring (also not helped by being unable to
create an fw batch via the backend UI because reasons, see
10ca096).

Fix this along most axis:

- Notify the user and reset the fw policy if the limit is updated
  while `fw=no`.
- Trigger a forward port if the fw policy is updated (from `no`) on a
  merged PR, currently only sources.
- Add check & warning to limit update process so it does *not* force a
  port (though maybe it should under the assumption that we're
  updating the limit anyway? We'll see).

Fixes #953

And unconditionally unstage when the HEAD of a PR is synchronised.

While a rebuild on a PR which was previously staged can be a false
positive (e.g. because it hit a non-derministic test the second time
around) it can also be legitimate e.g. auto-rebase of an old PR to
check it out. In that case the PR should be unstaged.

Furthermore, as soon as the PR gets rebuilt it goes back into
`approved` (because the status goes to pending and currently there's
no great way to suppress that in the rebuild case without also fucking
it up for the sync case). This would cause a sync on the PR to be
missed (in that it would not unstage the PR), which is broken. Fix
that by not checking the state of the PR beforehand, it seems to be an
unnecessary remnant of older code, and not really an optimisation (or
at least one likely not worth bothering with, especially as we then
proceed to perform a full PR validation anyway).

Fixes #950

- Switch to just `default` ci.
- Autouse fixture to init the master branch.
- Switch to `make_commits`.
- Merge `test_reopen_update` and `test_update_closed_revalidate` into
  `test_update_closed`: the former did basically nothing useful and
  the latter could easily be folded into `test_update_closed` by just
  validating the new commit.

By updating the staging timeout every time we run `_compute_state` and
still have a `pending` status, we'd actually bump the timeout *on
every success CI* except for the last one. Which was never the
intention and can add an hour or two to the mergebot-side timeout.

Instead, add an `updated_at` attribute to statuses (name taken from
the webhook payload even though we don't use that), read it when we
see `pending` required statuses, and update the staging timeout based
on that if necessary.

That way as long as we don't get *new* pending statuses, the timeout
doesn't get updated.

Fixes #952

Add warnings when trying to send comments / commands to PRs targeting
inactive branches.

This was missing leading to confusion, as one warning is clearly not
enough.

Fixes #941

Given branch A, and branch B forked from it. If B removes a file which
a PR to A later modifies, on forward port Git generates a
modify/delete conflict (as in one side modifies a file which the
other deleted).

So far so good, except while it does notify the caller of the issue
the modified file is just dumped as-is into the working copy (or
commit), which essentially resurrects it.

This is an issue, *especially* as the file is already part of a
commit (rather tan just a U local file), if the developer fixes the
conflict "in place" rather than re-doing the forward-port from scratch
it's easy to miss the reincarnated file (and possibly the changes that
were part of it too), which at best leaves parasitic dead code in the
working copy. There is also no easy way for the runbot to check it as
adding unimported standalone files while rare is not unknown
e.g. utility scripts (to say nothing of JS where we can't really track
the usages / imports at all).

To resolve this issue, during conflict generation post-process
modify/delete to insert artificial conflict markers, the file should
be syntactically invalid so linters / checkers should complain, and
the minimal check has a step looking for conflict markers which should
fire and prevent merging the PR.

Fixes #896

This is a dumb false positive, kill it.

- fix staging reasons containing escaped quotes (would render as
  ` ` to the end user)
- remove extra spacing around PR title @title/popovers
- simplify a few view conditionals through judicious use of `t-elif`
  and nesting
- make `staging_end` non-computed as it's not computed anymore, just
  set if and when the staging gets disabled
  (146564a)

Unstaged changes can be useful or necessary for some tasks
e.g. absolute emergency (where even faking the state of a staging is
not really desirable, if that's even possible anymore), or changes
which are so broad they're difficult to stage (e.g. t10s updates).

Add a new object which serves as a queue for patch to direct-apply,
with support for either text patches (udiff style out of git show or
format-patch) or commits to cherry-pick. In the former case, the part
of the show / format-patch before the diff itself is used for the
commit metadata (author, committer, dates, message) whereas for the
commit version the commit itself is reused as-is.

Applied patches are simply disabled for traceability.

Fixes #926

And use it in test_trivial_flow instead of the kinda half-assed manual
version.

Because of the false negatives due to github's reordering of events on
retargeting, blocking merge methods can be rather frustrating or the
user as what's happening and how to solve it isn't clear in that case.

Keep the warnings and all, but remove the blocking factor: if a PR
doesn't have a merge method and is not single-commit, just skip it on
staging. This way, PRs which are actually single-commit will stage
fine even if the mergebot thinks they shouldn't be.

Fixes #957

If a PR is closed but part of an ongoing batch, the change in status
of the batch might be reflected on the PR still:

- if a PR is closed and the batch gets staged, the PR shows up as
  being staged
- if the PR is merged then the batch gets merged, the PR shows up as
  merged

Fixes #914

Also remove the unused `_tagstate` helper property.

The mergebot tests have always been pretty gentle on system load which
is nice, however it's just looking at the list of longest tests that I
realised / re-membered the hook wait duration is 10 seconds for the
benefit of github, which doesn't really matter locally. This means on
interaction / cron-heavy tests the test might only be using on the
order of 10% CPU or something, that is a waste of time.

TBF this is easily compensated by increasing the concurrency of the
test suite (e.g. from 16 to 32 when I switched machine, but it seems
as if not more sensible to lower the webhook wait delay to something
more reasonable. 1s seems to be a good fit here, on my new computer at
n=16 it leads to the test suite running in 15mn at 600% CPU (which is
pretty good on a 6/12 CPU as it loads the system heavily but doesn't
completely bog it down).

Reducing it to 0.5s, the test suite takes the same duration but CPU
load increases to 770%, and errors creep up, likely a mix of
concurrency issues in the DB and dummy-central sending webhooks too
slowly as we compete with it for CPU resources (could actually make
sense to restrict the number of threads tokio can use). Reducing
concurrency could make this work better, but I think at this point
we're in a pretty good state, it's even somewhat reasonable to run the
test suite sequentially (taking about 1h10 but being functionally
invisible in terms of load).

Missed it during the previous pass, probably because it's in the
middle of `pull_requests.py`. It's a classic template for triggered
crons since the model is just a queue of actions for the cron.

- replace manual token_urlsafe by actual token_urlsafe
- make conditional right side up and more readable
- replace match by fullmatch, should not change anything since we end
  with a greedy universal match but is slightly more explicit

This is not a full user-driven backport thingie for now, just one
admins can use to facilitate thing and debug issues with the
system. May eventually graduate to a frontend feature.

Fixes #925

For the longest time Github's `change` key was borked when
transitioning a description to and from empty. They fixed that during
2023, which I already saw and impacted on
DC (xmo-odoo/dummy_central@1ebed9d
and xmo-odoo/dummy_central@937e87c)
but this had yet to be taken in account by the mergebot.

This is now done, the code is functionally reverted to what it was
before I realised `changes` was hosed and moved off of it in
3da1874.

Fixes #743

As far as I can tell they are properly handled:

- In `handle_status` we let the http layer retry the query, which
  pretty much always succeeds.
- In `Commit.notify`, we rollback the application of the current
  commit, meaning it'll be processed by the next run of the cron,
  which also seems to succeed every time (that is going through the
  log I pretty much never notice the same commit being serialization
  failure'd twice in a row).

  Which we can trigger for faster action, this last item is not
  entirely necessary as statuses should generally come in fast and
  especially if we have concurrency errors, but it can't hurt.

This means the only genuine issue is... sql_db logging a "bad query"
every time there's a serialization failure.

In `handle_status`, just suppress the message outright, if there's an
error other than serialization the http / dispatch layer should catch
and log it.

In `Commit._notify` things are slightly more difficult as the execute
is implicit (`flush` -> `_write` -> `execute`) so we can't pass the
flag by parameter. One option would be to set and unset
`_default_log_exception`, but it would either be a bit dodgy or it
would require using a context manager and increasing the indentation
level (or using a custom context manager).

Instead just `mute_logger` the fucking thing. It's a bit brutish and
mostly used in tests, but not just, and feels like the least bad
option here...

Closes #805

Like limit, fw=no should restrict the table length, in this case to
just the current branch (as we're not forward porting at all).

Before this, `no` would not be applied as a limit visually, the table
would still go up to the main branch which is very confusing.

Fixes #962

pymarkdown's footnotes plugin *saves footnotes across invocations by
default*. Even if I understand the documented use case it seems wild
that it's not opt-in...

Anyway disable that resetting all internal state. Thanks rfr for the
inital report that things were looking odd.

- fix incorrect view specs (the action id comes first)
- add a wizard form and hook it into the PR, completely forgot to do
  that
- usability improvements: filter branches to be in the same project as
  the PR being backported, and older than the current PR's branch

The latter is a somewhat incomplete condition: ideally we'd want to
only allow selecting branches preceding the target of the *source* of
the PR being backported, that way we don't risk errors when
backporting forward-ports (the condition should be checked in the
final action but still).

Also we're only filtering by sequence, so we're missing the name part
of the ordering, hence if multiple branches have the same sequence we
may not allow selecting some of the preceding branches.

Turns out you can configure format-patch with `--no-prefix` and some
people (*cough cough* mat) have that in their standard setup, so the
assumption of needing to strip 1 level of prefix does not necessarily
hold.

Also fix a few more issues:

- some people (*cough cough* still mat) also use `-n` by default,
  which adds the series sequence (`n/m`) even for a single patch,
  handle that correctly
- logging patch application errors is pretty useful when patching
  fails and I'm trying to get the information via logs, do that
- especially when I decide to add error messages to tracking *but
  forgot to show the chatter by default*, fix that as well

The commit-based patcher worked first try, and patch-based would have
worked too if not for those meddling kids. In the future it might be a
good idea to reify the stripping level (`-p`) on the patch object
though, and maybe provide a computed preview of the list of files to
patch, so issues are easier for the operator to diagnose.

If staging gets re-enabled on a branch (or the branch itself gets
re-enabled), immediately run a staging cron as there may already be
PRs waiting, and no trigger enqueued: cron triggers have no payload,
they just get removed when the cron runs which means if a bunch of PRs
become ready for branch B with staging disabled, the cron is going to
run, it's going to stage nothing on that branch (because staging is
disabled) then it's going to delete all the triggers.

Fixes #979

If a comment causes an unknown PR to be fetched, it's a bit odd to
ping the author (and possibly reviewer) anyway as they're not super
concerned (and technically we could be ignoring the purported /
attempted reviewer).

So if a fetch job was created because of a comment, remember the
comment author and ping *them* instead of using the default ping
policy.

Fixes #981

Since b45ecf0 forwardport batches
which fail have a delay set in order to avoid spamming. However that
delay was not displayed anywhere, which made things confusing as the
batch would not get run even after creating new triggers.

Show the delay if it's set (to a value later than now), as a relative
delta for clarity (as normally the delay is in minutes so a full blown
date is difficult to read / aprehend), and allow viewing and setting
it in the form view.

Fixes #982

Add a few views / view extensions set as custom on the production
mergebot which I never remembered to implement in the actual source.

If a branch is created via SQL directly, one might forget to set its
`write_date`, leading the computation of the `last_modified` to be
*really* unhappy as it's asked to compare a bunch of datetime objects
to `False` in order to compute the `max`.

Substitute missing `write_date` with `datetime.min` so they are
extremely unlikely to influence the computation until and unless the
branch gets updated.

`get_content` round-trips the text part through `ascii` with
`error=replace`, so if the input is not ascii it screws up
tremendously, which leads to either failing to apply patches (the more
likely situation) or corrupting the patches.

`get_payload`, if called without `decode`, pretty much just returns
the payload unless it needs a decoding pass (e.g. because it contains
raw surrogates, but that should not be an issue for us). So this is
really what we want.

While at it, increase `patch`'s verbosity in case it can give us more
info.

Got a bunch of updates since the initial attempt to migrate the
mergebot before the odoo days.

Update a bunch of `create` overrides to work in batch. Also fix a few
`super()` calls unnecessarily in legacy style.

Got a bunch of updates since the initial attempt to migrate the
mergebot before the odoo days.

Not sure why it didn't break tests in 16...

`read_tracking_value` likely never worked correctly in both branches,
but worked in 15.0 because the failures to do anything useful happened
to end in the right case?

Because of tracking, `test_patch_acl` needs access to message subtypes
during patch creation. If the user *only* has
`runbot_merge.group_patcher` or `runbot_merge.group_admin` they don't
have any of the "core" user groups (public, portal, internal) and thus
don't have access to mail subtypes.

Fix that by having the runbot_merge groups imply being an internal
user.

Basically the next part of aa1df22
which requires replacing @attrs by the corresponding attribute &
python predicates: new attrs were added to 15.0 since.

Apparently in odoo 17.0 tracking values are always ordered by field
name.

Before this, if creating the DB failed the next worker would find
themselves with an empty `template-` file, so they would take the path
to *create* a template database, which would fail when trying to
`mkdir` the `shared-` directory as the directory would already exist.

The problem with this is this module would likely immediately fail and
take down their worker, triggering a test suite failure for themselves
and likely hiding the *true* failure cause (not sure why the
originally failed worker isn't the first one to trigger a failure but
there you go).

By skipping the tests instead, we provide a lot more opportunity for
the "true" failure to be revealed.

Can't be arsed to rebase and re-resolve the conflicts, so remerge
instead.

`runbot_merge.patch` is a mail.thread model added since the last
migration attempt, see 36786d5

The docker metadata are currently computed only on
some images durring the nightly.

This aims to get metadata after each docker image build in order to be
able to rely on them when needed.

Previous fix was not enough when one of the args is not a string.

Courtesy of xmo

When digging into deactivated build errors, one cannot easily find why
an error was deactivated and to which one it was merged.

With this commit, a message is added in the chatter to explain where it
is merged.

Duplicate error content should not happens ... but it does.
With this commit, a server actions allows to relink error contents and
thus removes error contents having the same fingerprint.