[FIX] forwardport: don't break forward porting on huge conflicts
On forward-porting, odoo/odoo#170183 generates a conflict on pretty
much every one of the 1111 files it touches, because they are
modify/delete conflicts that generates a conflict message over 200
bytes per file, which is over 200kB of output.
For this specific scenario, the commit message was being passed
through arguments to the `git` command, resulting in a command line
exceeding `MAX_ARG_STRLEN`[^1]. The obvious way to fix this is to pass
the commit message via stdin as is done literally in the line above
where we just copy a non-generated commit message.
However I don't think hundreds of kbytes worth of stdout[^2] is of any
use, so shorten that a bit, and stderr while at it.
Don't touch the commit message size for now, possibly forever, but
note that some log diving reveals a commit with a legit 18kB message
(odoo/odoo@42a3b704f7c7889a74338253138d0201d3b6e7a3) so if we want to
restrict that the limit should be at least 32k, and possibly 64. But
it might be a good idea to make that limit part of the ready / merge
checks too, rather than cut things off or trigger errors during
staging.
Fixes #900
[^1]: Most resources on "Argument list too long" reference `ARG_MAX`,
but on both my machine and the server it is 2097152 (25% of the
default stack), which is ~10x larger than the commit message we
tried to generate. The actual limit is `MAX_ARG_STRLEN` which
can't be queried directly but is essentially hard-coded to
PAGE_SIZE * 32 = 128kiB, which tracks.
[^2]: Somewhat unexpectedly, that's where `git-cherry-pick` sends the
conflict info.
2024-06-25 18:11:32 +07:00
|
|
|
import random
|
2021-08-02 14:18:30 +07:00
|
|
|
import re
|
|
|
|
import time
|
|
|
|
from operator import itemgetter
|
|
|
|
|
2024-06-11 20:41:03 +07:00
|
|
|
import pytest
|
|
|
|
|
2024-06-04 17:15:29 +07:00
|
|
|
from utils import make_basic, Commit, validate_all, matches, seen, REF_PATTERN, to_pr
|
2021-08-02 14:18:30 +07:00
|
|
|
|
|
|
|
|
|
|
|
def test_conflict(env, config, make_repo, users):
|
|
|
|
""" Create a PR to A which will (eventually) conflict with C when
|
|
|
|
forward-ported.
|
|
|
|
"""
|
|
|
|
prod, other = make_basic(env, config, make_repo)
|
|
|
|
# create a d branch
|
|
|
|
with prod:
|
|
|
|
prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d')
|
|
|
|
project = env['runbot_merge.project'].search([])
|
|
|
|
project.write({
|
|
|
|
'branch_ids': [
|
2023-06-08 13:19:43 +07:00
|
|
|
(0, 0, {'name': 'd', 'sequence': 40})
|
2021-08-02 14:18:30 +07:00
|
|
|
]
|
|
|
|
})
|
|
|
|
|
|
|
|
# generate a conflict: create a h file in a PR to a
|
|
|
|
with prod:
|
|
|
|
[p_0] = prod.make_commits(
|
|
|
|
'a', Commit('p_0', tree={'h': 'xxx'}),
|
|
|
|
ref='heads/conflicting'
|
|
|
|
)
|
|
|
|
pr = prod.make_pr(target='a', head='conflicting')
|
|
|
|
prod.post_status(p_0, 'success', 'legal/cla')
|
|
|
|
prod.post_status(p_0, 'success', 'ci/runbot')
|
|
|
|
pr.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
prod.post_status('staging.a', 'success', 'legal/cla')
|
|
|
|
prod.post_status('staging.a', 'success', 'ci/runbot')
|
|
|
|
env.run_crons()
|
|
|
|
pra_id, prb_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
# mark pr b as OK so it gets ported to c
|
|
|
|
with prod:
|
|
|
|
validate_all([prod], [prb_id.head])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
pra_id, prb_id, prc_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
# should have created a new PR
|
|
|
|
# but it should not have a parent, and there should be conflict markers
|
|
|
|
assert not prc_id.parent_id
|
|
|
|
assert prc_id.source_id == pra_id
|
|
|
|
assert prc_id.state == 'opened'
|
|
|
|
|
|
|
|
p = prod.commit(p_0)
|
2023-08-30 17:10:46 +07:00
|
|
|
prc = prod.get_pr(prc_id.number)
|
2021-08-02 14:18:30 +07:00
|
|
|
c = prod.commit(prc_id.head)
|
|
|
|
assert c.author == p.author
|
|
|
|
# ignore date as we're specifically not keeping the original's
|
|
|
|
without_date = itemgetter('name', 'email')
|
|
|
|
assert without_date(c.committer) == without_date(p.committer)
|
|
|
|
assert prod.read_tree(c) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'a',
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
'h': matches('''<<<\x3c<<< $$
|
2021-08-02 14:18:30 +07:00
|
|
|
a
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
||||||| $$
|
2021-08-02 14:18:30 +07:00
|
|
|
=======
|
|
|
|
xxx
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
>>>\x3e>>> $$
|
2021-08-02 14:18:30 +07:00
|
|
|
'''),
|
|
|
|
}
|
2023-08-30 17:10:46 +07:00
|
|
|
assert prc.comments == [
|
|
|
|
seen(env, prc, users),
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
(users['user'],
|
2024-06-04 17:15:29 +07:00
|
|
|
f'''@{users['user']} @{users['reviewer']} cherrypicking of pull request {pra_id.display_name} failed.
|
2023-08-30 17:10:46 +07:00
|
|
|
|
|
|
|
stdout:
|
|
|
|
```
|
|
|
|
Auto-merging h
|
2024-06-04 17:15:29 +07:00
|
|
|
CONFLICT (add/add): Merge conflict in h
|
2023-08-30 17:10:46 +07:00
|
|
|
|
|
|
|
```
|
|
|
|
|
2024-06-04 17:15:29 +07:00
|
|
|
Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
|
2023-08-30 17:10:46 +07:00
|
|
|
|
2024-06-04 17:15:29 +07:00
|
|
|
In the former case, you may want to edit this PR message as well.
|
2023-08-30 17:10:46 +07:00
|
|
|
|
2024-06-04 17:15:29 +07:00
|
|
|
:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}.
|
2023-08-30 17:10:46 +07:00
|
|
|
|
2024-06-04 17:15:29 +07:00
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
[CHG] forwardport: perform forward porting without working copies
The goal is to reduce maintenance and odd disk interactions &
concurrency issues, by not creating concurrent clones, not having to
push forks back in the repository, etc... it also removes the need to
cleanup "scratch" working copies though that looks not to have been an
issue in a while.
The work is done on isolated objects without using or mutating refs,
so even concurrent work should not be a problem.
This turns out to not be any more verbose (less so if anything) than
using `cherry-pick`, as that is not really designed for scripted /
non-interactive use, or for squashing commits thereafter. Working
directly with trees and commits is quite a bit cleaner even without a
ton of helpers.
Much of the credit goes to Julia Evans for [their investigation of
3-way merges as the underpinnings of cherry-picking][3-way merge],
this would have been a lot more difficult if I'd had to rediscover the
merge-base trick independently.
A few things have been changed by this:
- The old trace/stderr from cherrypick has disappeared as it's
generated by cherrypick, but for a non-interactive use it's kinda
useless anyway so I probably should have looked into removing it
earlier (I think the main use was investigation of the inflateinit
issue).
- Error on emptied commits has to be hand-rolled as `merge-tree`
couldn't care less, this is not hard but is a bit annoying.
- `merge-tree`'s conflict information only references raw commits,
which makes sense, but requires updating a bunch of tests. Then
again so does the fact that it *usually* doesn't send anything to
stderr, so that's usually disappearing.
Conveniently `merge-tree` merges the conflict marker directly in the
files / tree so we don't have to mess about moving them back out of
the repository and into the working copy as I assume cherry-pick does,
which means we don't have to try and commit them back in ether. That
is a huge part of the gain over faffing about with the working copy.
Fixes #847
[3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/
2024-07-05 18:32:02 +07:00
|
|
|
''')
|
2023-08-30 17:10:46 +07:00
|
|
|
]
|
|
|
|
|
2021-08-02 14:18:30 +07:00
|
|
|
prb = prod.get_pr(prb_id.number)
|
|
|
|
assert prb.comments == [
|
|
|
|
seen(env, prb, users),
|
|
|
|
(users['user'], '''\
|
|
|
|
This PR targets b and is part of the forward-port chain. Further PRs will be created up to d.
|
|
|
|
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
|
|
'''),
|
2022-06-23 19:25:07 +07:00
|
|
|
(users['user'], """@%s @%s the next pull request (%s) is in conflict. \
|
|
|
|
You can merge the chain up to here by saying
|
[CHG] *: rewrite commands set, rework status management
This commit revisits the commands set in order to make it more
regular, and limit inconsistent command-sets, although it includes
pseudo-command aliases for common tasks now removed from the core set.
Hard Errors
===========
The previous iteration of the commands set would ignore any
non-command term in a command line. This has been changed to hard
error (and ignoring the entire thing) if any command is unknown or
invalid.
This fixes inconsistent / unexpected interpretations where a user
sends a command, then writes a novel on the same line some words of
which happen to *also* be commands, leading to merge states they did
not expect. They should now be told to fuck off.
Priority Restructuring
----------------------
The numerical priority system was pretty messy in that it confused
"staging priority" (in ways which were not entirely straightforward)
with overrides to other concerns.
This has now being split along all the axis, with separate command
subsets for:
- staging prioritisation, now separated between `default`, `priority`,
and `alone`,
- `default` means PRs are picked by an unspecified order when
creating a staging, if nothing better is available
- `priority` means PRs are picked first when staging, however if
`priority` PRs don't fill the staging the rest will be filled with
`default`, this mode did not previously exist
- `alone` means the PRs are picked first, before splits, and only
`alone` PRs can be part of the staging (which usually matches the
modename)
- `skipchecks` overrides both statuses and approval checks, for the
batch, something previously implied in `p=0`, but now
independent. Setting `skipchecks` basically makes the entire batch
`ready`.
For consistency this also sets the reviewer implicitly: since
skipchecks overrides both statuses *and approval*, whoever enables
this mode is essentially the reviewer.
- `cancel` cancels any ongoing staging when the marked PR becomes
ready again, previously this was also implied (in a more restricted
form) by setting `p=0`
FWBot removal
=============
While the "forwardport bot" still exists as an API level (to segregate
access rights between tokens) it has been removed as an interaction
point, as part of the modules merge plan. As a result,
fwbot stops responding
----------------------
Feedback messages are now always sent by the mergebot, the
forward-porting bot should not send any message or notification
anymore.
commands moved to the merge bot
-------------------------------
- `ignore`/`up to` simply changes bot
- `close` as well
- `skipci` is now a choice / flag of an `fw` command, which denotes
the forward-port policy,
- `fw=default` is the old `ci` and resets the policy to default,
that is wait for the PR to be merged to create forward ports, and
for the required statuses on each forward port to be received
before creating the next
- `fw=skipci` is the old `skipci`, it waits for the merge of the
base PR but then creates all the forward ports immediately (unless
it gets a conflict)
- `fw=skipmerge` immediately creates all the forward ports, without
even waiting for the PR to be merged
This is a completely new mode, and may be rather broken as until
now the 'bot has always assumed the source PR had been merged.
approval rework
---------------
Because of the previous section, there is no distinguishing feature
between `mergebot r+` = "merge this PR" and `forwardbot r+` = "merge
this PR and all its parent with different access rights".
As a result, the two have been merged under a single `mergebot r+`
with heuristics attempting to provide the best experience:
- if approving a non-forward port, the behavior does not change
- else, with review rights on the source, all ancestors are approved
- else, as author of the original, approves all ancestors which descend
from a merged PR
- else, approves all ancestors up to and including the oldest ancestor
to which we have review rights
Most notably, the source's author is not delegated on the source or
any of its descendants anymore. This might need to be revisited if it
provides too restrictive.
For the very specialized need of approving a forward-port *and none of
its ancestors*, `review=` can now take a comma (`,`) separated list of
pull request numbers (github numbers, not mergebot ids).
Computed State
==============
The `state` field of pull requests is now computed. Hopefully this
makes the status more consistent and predictable in the long run, and
importantly makes status management more reliable (because reference
datum get updated naturally flowing to the state).
For now however it makes things more complicated as some of the states
have to be separately signaled or updated:
- `closed` and `error` are now separate flags
- `merge_date` is pulled down from forwardport and becomes the
transition signal for ready -> merged
- `reviewed_by` becomes the transition signal for approval (might be a
good idea to rename it...)
- `status` is computed from the head's statuses and overrides, and
*that* becomes the validation state
Ideally, batch-level flags like `skipchecks` should be on, well, the
batch, and `state` should have a dependency on the batch. However
currently the batch is not a durable / permanent member of the system,
so it's a PR-level flag and a messy pile.
On notable change is that *forcing* the state to `ready` now does that
but also sets the reviewer, `skipchecks`, and overrides to ensure the
API-mediated readying does not get rolled back by e.g. the runbot
sending a status.
This is useful for a few types of automated / programmatic PRs
e.g. translation exports, where we set the state programmatically to
limit noise.
recursive dependency hack
-------------------------
Given a sequence of PRs with an override of the source, if one of the
PRs is updated its descendants should not have the override
anymore. However if the updated PR gets overridden, its descendants
should have *that* override.
This requires some unholy manipulations via an override of `modified`,
as the ORM supports recursive fields but not recursive
dependencies (on a different field).
unconditional followup scheduling
---------------------------------
Previously scheduling forward-port followup was contigent on the FW
policy, but it's not actually correct if the new PR is *immediately*
validated (which can happen now that the field is computed, if there
are no required statuses *or* all of the required statuses are
overridden by an ancestor) as nothing will trigger the state change
and thus scheduling of the fp followup.
The followup function checks all the properties of the batch to port,
so this should not result on incorrect ports. Although it's a bit more
expensive, and will lead to more spam.
Previously this would not happen because on creation of a PR the
validation task (commit -> PR) would still have to execute.
Misc Changes
============
- If a PR is marked as overriding / canceling stagings, it now does
so on retry not just when setting initially.
This was not handled at all previously, so a PR in P0 going into
error due to e.g. a non-deterministic bug would be retried and still
p=0, but a current staging would not get cancelled. Same when a PR
in p=0 goes into error because something was failed, then is updated
with a fix.
- Add tracking to a bunch of relevant PR fields.
Post-mortem analysis currently generally requires going through the
text logs to see what happened, which is annoying.
There is a nondeterminism / inconsistency in the tracking which
sometimes leads the admin user to trigger tracking before the bot
does, leading to the staging tracking being attributed to them
during tests, shove under the carpet by ignoring the user to whom
that tracking is attributed.
When multiple users update tracked fields in the same transaction
all the changes are attributed to the first one having triggered
tracking (?), I couldn't find why the admin sometimes takes over.
- added and leveraged support for enum-backed selection fields
- moved variuous fields from forwardport to runbot_merge
- fix a migration which had never worked and which never run (because
I forgot to bump the version on the module)
- remove some unnecessary intermediate de/serialisation
fixes #673, fixes #309, fixes #792, fixes #846 (probably)
2023-10-31 13:42:07 +07:00
|
|
|
> @hansen r+
|
2021-08-02 14:18:30 +07:00
|
|
|
|
|
|
|
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
|
|
|
|
""" % (
|
|
|
|
users['user'], users['reviewer'],
|
|
|
|
prc_id.display_name,
|
|
|
|
))
|
|
|
|
]
|
|
|
|
|
|
|
|
# check that CI passing does not create more PRs
|
|
|
|
with prod:
|
|
|
|
validate_all([prod], [prc_id.head])
|
|
|
|
env.run_crons()
|
|
|
|
time.sleep(5)
|
|
|
|
env.run_crons()
|
|
|
|
assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\
|
2021-08-11 16:36:35 +07:00
|
|
|
"CI passing should not have resumed the FP process on a conflicting PR"
|
2021-08-02 14:18:30 +07:00
|
|
|
|
|
|
|
# fix the PR, should behave as if this were a normal PR
|
|
|
|
prc = prod.get_pr(prc_id.number)
|
|
|
|
pr_repo, pr_ref = prc.branch
|
|
|
|
with pr_repo:
|
|
|
|
pr_repo.make_commits(
|
|
|
|
# if just given a branch name, goes and gets it from pr_repo whose
|
|
|
|
# "b" was cloned before that branch got rolled back
|
|
|
|
'c',
|
|
|
|
Commit('h should indeed be xxx', tree={'h': 'xxx'}),
|
|
|
|
ref='heads/%s' % pr_ref,
|
|
|
|
make=False,
|
|
|
|
)
|
|
|
|
env.run_crons()
|
|
|
|
assert prod.read_tree(prod.commit(prc_id.head)) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'a',
|
|
|
|
'h': 'xxx',
|
|
|
|
}
|
|
|
|
assert prc_id.state == 'opened', "state should be open still"
|
|
|
|
assert ('#%d' % pra_id.number) in prc_id.message
|
|
|
|
|
|
|
|
# check that merging the fixed PR fixes the flow and restarts a forward
|
|
|
|
# port process
|
|
|
|
with prod:
|
|
|
|
prod.post_status(prc.head, 'success', 'legal/cla')
|
|
|
|
prod.post_status(prc.head, 'success', 'ci/runbot')
|
|
|
|
prc.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
assert prc_id.staging_id
|
|
|
|
with prod:
|
|
|
|
prod.post_status('staging.c', 'success', 'legal/cla')
|
|
|
|
prod.post_status('staging.c', 'success', 'ci/runbot')
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
*_, prd_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
assert ('#%d' % pra_id.number) in prd_id.message, \
|
|
|
|
"check that source / PR A is referenced by resume PR"
|
|
|
|
assert ('#%d' % prc_id.number) in prd_id.message, \
|
|
|
|
"check that parent / PR C is referenced by resume PR"
|
|
|
|
assert prd_id.parent_id == prc_id
|
|
|
|
assert prd_id.source_id == pra_id
|
|
|
|
assert re.match(
|
|
|
|
REF_PATTERN.format(target='d', source='conflicting'),
|
|
|
|
prd_id.refname
|
|
|
|
)
|
|
|
|
assert prod.read_tree(prod.commit(prd_id.head)) == {
|
|
|
|
'f': 'c',
|
|
|
|
'g': 'a',
|
|
|
|
'h': 'xxx',
|
|
|
|
'i': 'a',
|
|
|
|
}
|
|
|
|
|
[FIX] forwardport: don't break forward porting on huge conflicts
On forward-porting, odoo/odoo#170183 generates a conflict on pretty
much every one of the 1111 files it touches, because they are
modify/delete conflicts that generates a conflict message over 200
bytes per file, which is over 200kB of output.
For this specific scenario, the commit message was being passed
through arguments to the `git` command, resulting in a command line
exceeding `MAX_ARG_STRLEN`[^1]. The obvious way to fix this is to pass
the commit message via stdin as is done literally in the line above
where we just copy a non-generated commit message.
However I don't think hundreds of kbytes worth of stdout[^2] is of any
use, so shorten that a bit, and stderr while at it.
Don't touch the commit message size for now, possibly forever, but
note that some log diving reveals a commit with a legit 18kB message
(odoo/odoo@42a3b704f7c7889a74338253138d0201d3b6e7a3) so if we want to
restrict that the limit should be at least 32k, and possibly 64. But
it might be a good idea to make that limit part of the ready / merge
checks too, rather than cut things off or trigger errors during
staging.
Fixes #900
[^1]: Most resources on "Argument list too long" reference `ARG_MAX`,
but on both my machine and the server it is 2097152 (25% of the
default stack), which is ~10x larger than the commit message we
tried to generate. The actual limit is `MAX_ARG_STRLEN` which
can't be queried directly but is essentially hard-coded to
PAGE_SIZE * 32 = 128kiB, which tracks.
[^2]: Somewhat unexpectedly, that's where `git-cherry-pick` sends the
conflict info.
2024-06-25 18:11:32 +07:00
|
|
|
def test_massive_conflict(env, config, make_repo):
|
|
|
|
"""If the conflict is large enough, the commit message may exceed ARG_MAX
|
|
|
|
and trigger E2BIG.
|
|
|
|
"""
|
|
|
|
# CONFLICT (modify/delete): <file> deleted in <commit> (<title>) and modified in HEAD. Version HEAD of <file> left in tree.
|
|
|
|
#
|
|
|
|
# 107 + 2 * len(filename) + len(title) per conflicting file.
|
|
|
|
# - filename: random.randbytes(10).hex() -> 20
|
|
|
|
# - title: random.randbytes(20).hex() -> 40
|
|
|
|
# -> 701 (!) files
|
|
|
|
|
|
|
|
files = []
|
|
|
|
while len(files) < 1500:
|
|
|
|
files.append(random.randbytes(10).hex())
|
|
|
|
|
|
|
|
# region setup
|
|
|
|
project = env['runbot_merge.project'].create({
|
|
|
|
'name': "thing",
|
|
|
|
'github_token': config['github']['token'],
|
|
|
|
'github_prefix': 'hansen',
|
2024-11-29 15:04:06 +07:00
|
|
|
'github_name': config['github']['name'],
|
|
|
|
'github_email': "foo@example.org",
|
[FIX] forwardport: don't break forward porting on huge conflicts
On forward-porting, odoo/odoo#170183 generates a conflict on pretty
much every one of the 1111 files it touches, because they are
modify/delete conflicts that generates a conflict message over 200
bytes per file, which is over 200kB of output.
For this specific scenario, the commit message was being passed
through arguments to the `git` command, resulting in a command line
exceeding `MAX_ARG_STRLEN`[^1]. The obvious way to fix this is to pass
the commit message via stdin as is done literally in the line above
where we just copy a non-generated commit message.
However I don't think hundreds of kbytes worth of stdout[^2] is of any
use, so shorten that a bit, and stderr while at it.
Don't touch the commit message size for now, possibly forever, but
note that some log diving reveals a commit with a legit 18kB message
(odoo/odoo@42a3b704f7c7889a74338253138d0201d3b6e7a3) so if we want to
restrict that the limit should be at least 32k, and possibly 64. But
it might be a good idea to make that limit part of the ready / merge
checks too, rather than cut things off or trigger errors during
staging.
Fixes #900
[^1]: Most resources on "Argument list too long" reference `ARG_MAX`,
but on both my machine and the server it is 2097152 (25% of the
default stack), which is ~10x larger than the commit message we
tried to generate. The actual limit is `MAX_ARG_STRLEN` which
can't be queried directly but is essentially hard-coded to
PAGE_SIZE * 32 = 128kiB, which tracks.
[^2]: Somewhat unexpectedly, that's where `git-cherry-pick` sends the
conflict info.
2024-06-25 18:11:32 +07:00
|
|
|
'fp_github_token': config['github']['token'],
|
|
|
|
'fp_github_name': 'herbert',
|
|
|
|
'branch_ids': [
|
|
|
|
(0, 0, {'name': 'a', 'sequence': 100}),
|
|
|
|
(0, 0, {'name': 'b', 'sequence': 80}),
|
|
|
|
],
|
|
|
|
})
|
|
|
|
|
|
|
|
repo = make_repo("repo")
|
|
|
|
env['runbot_merge.events_sources'].create({'repository': repo.name})
|
|
|
|
|
|
|
|
repo_id = env['runbot_merge.repository'].create({
|
|
|
|
'project_id': project.id,
|
|
|
|
'name': repo.name,
|
|
|
|
'required_statuses': "default",
|
|
|
|
'fp_remote_target': repo.name,
|
|
|
|
'group_id': False,
|
|
|
|
})
|
|
|
|
env['res.partner'].search([
|
|
|
|
('github_login', '=', config['role_reviewer']['user'])
|
|
|
|
]).write({
|
|
|
|
'review_rights': [(0, 0, {'repository_id': repo_id.id, 'review': True})]
|
|
|
|
})
|
|
|
|
|
|
|
|
with repo:
|
|
|
|
# create branch with a ton of empty files
|
|
|
|
repo.make_commits(
|
|
|
|
None,
|
|
|
|
Commit(
|
|
|
|
random.randbytes(20).hex(),
|
|
|
|
tree=dict.fromkeys(files, "xoxo"),
|
|
|
|
),
|
|
|
|
ref='heads/a',
|
|
|
|
)
|
|
|
|
|
|
|
|
# removes all those files in the next branch
|
|
|
|
repo.make_commits(
|
|
|
|
'a',
|
|
|
|
Commit(
|
|
|
|
random.randbytes(20).hex(),
|
|
|
|
tree=dict.fromkeys(files, "content!"),
|
|
|
|
),
|
|
|
|
ref='heads/b',
|
|
|
|
)
|
|
|
|
# endregion setup
|
|
|
|
|
|
|
|
with repo:
|
|
|
|
# update all the files
|
|
|
|
repo.make_commits(
|
|
|
|
'a',
|
|
|
|
Commit(random.randbytes(20).hex(), tree={'a': '1'}),
|
|
|
|
Commit(random.randbytes(20).hex(), tree={'x': '1'}, reset=True),
|
|
|
|
ref='heads/change',
|
|
|
|
)
|
|
|
|
pr = repo.make_pr(target='a', head='change')
|
|
|
|
repo.post_status('refs/heads/change', 'success')
|
|
|
|
pr.post_comment('hansen rebase-ff r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
with repo:
|
|
|
|
repo.post_status('staging.a', 'success')
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
# we don't actually need more, the bug crashes the forward port entirely so
|
|
|
|
# the PR is never even created
|
|
|
|
_pra_id, _prb_id = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
|
|
|
|
|
2021-08-02 14:18:30 +07:00
|
|
|
def test_conflict_deleted(env, config, make_repo):
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
prod, other = make_basic(env, config, make_repo, statuses="default")
|
2021-08-02 14:18:30 +07:00
|
|
|
# remove f from b
|
|
|
|
with prod:
|
|
|
|
prod.make_commits(
|
|
|
|
'b', Commit('33', tree={'g': 'c'}, reset=True),
|
|
|
|
ref='heads/b'
|
|
|
|
)
|
|
|
|
|
|
|
|
# generate a conflict: update f in a
|
|
|
|
with prod:
|
|
|
|
[p_0] = prod.make_commits(
|
|
|
|
'a', Commit('p_0', tree={'f': 'xxx'}),
|
|
|
|
ref='heads/conflicting'
|
|
|
|
)
|
|
|
|
pr = prod.make_pr(target='a', head='conflicting')
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
prod.post_status(p_0, 'success')
|
2021-08-02 14:18:30 +07:00
|
|
|
pr.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
|
|
|
|
env.run_crons()
|
|
|
|
with prod:
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
prod.post_status('staging.a', 'success')
|
2021-08-02 14:18:30 +07:00
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
# should have created a new PR
|
|
|
|
pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
# but it should not have a parent
|
|
|
|
assert not pr1.parent_id
|
|
|
|
assert pr1.source_id == pr0
|
|
|
|
assert prod.read_tree(prod.commit('b')) == {
|
|
|
|
'g': 'c',
|
|
|
|
}
|
|
|
|
assert pr1.state == 'opened'
|
|
|
|
assert prod.read_tree(prod.commit(pr1.head)) == {
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
'f': matches("""\
|
|
|
|
<<<\x3c<<< $$
|
|
|
|
||||||| $$
|
|
|
|
=======
|
|
|
|
xxx
|
|
|
|
>>>\x3e>>> $$
|
|
|
|
"""),
|
2021-08-02 14:18:30 +07:00
|
|
|
'g': 'c',
|
|
|
|
}
|
|
|
|
|
|
|
|
# check that CI passing does not create more PRs
|
|
|
|
with prod:
|
|
|
|
validate_all([prod], [pr1.head])
|
|
|
|
env.run_crons()
|
|
|
|
time.sleep(5)
|
|
|
|
env.run_crons()
|
|
|
|
assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\
|
2021-08-11 16:36:35 +07:00
|
|
|
"CI passing should not have resumed the FP process on a conflicting PR"
|
2021-08-02 14:18:30 +07:00
|
|
|
|
|
|
|
# fix the PR, should behave as if this were a normal PR
|
|
|
|
get_pr = prod.get_pr(pr1.number)
|
|
|
|
pr_repo, pr_ref = get_pr.branch
|
|
|
|
with pr_repo:
|
|
|
|
pr_repo.make_commits(
|
|
|
|
# if just given a branch name, goes and gets it from pr_repo whose
|
|
|
|
# "b" was cloned before that branch got rolled back
|
|
|
|
prod.commit('b').id,
|
|
|
|
Commit('f should indeed be removed', tree={'g': 'c'}, reset=True),
|
|
|
|
ref='heads/%s' % pr_ref,
|
|
|
|
make=False,
|
|
|
|
)
|
|
|
|
env.run_crons()
|
|
|
|
assert prod.read_tree(prod.commit(pr1.head)) == {
|
|
|
|
'g': 'c',
|
|
|
|
}
|
|
|
|
assert pr1.state == 'opened', "state should be open still"
|
|
|
|
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
|
|
|
|
def test_conflict_deleted_deep(env, config, make_repo):
|
|
|
|
""" Same as the previous one but files are deeper than toplevel, and we only
|
|
|
|
want to see if the conflict post-processing works.
|
|
|
|
"""
|
|
|
|
# region: setup
|
|
|
|
prod = make_repo("test")
|
|
|
|
env['runbot_merge.events_sources'].create({'repository': prod.name})
|
|
|
|
with prod:
|
|
|
|
[a, b] = prod.make_commits(
|
|
|
|
None,
|
|
|
|
Commit("a", tree={
|
|
|
|
"foo/bar/baz": "1",
|
|
|
|
"foo/bar/qux": "1",
|
|
|
|
"corge/grault": "1",
|
|
|
|
}),
|
|
|
|
Commit("b", tree={"foo/bar/qux": "2"}, reset=True),
|
|
|
|
)
|
|
|
|
prod.make_ref("heads/a", a)
|
|
|
|
prod.make_ref("heads/b", b)
|
|
|
|
|
|
|
|
project = env['runbot_merge.project'].create({
|
|
|
|
'name': "test",
|
|
|
|
'github_token': config['github']['token'],
|
|
|
|
'github_prefix': 'hansen',
|
2024-11-29 15:04:06 +07:00
|
|
|
'github_name': config['github']['name'],
|
|
|
|
'github_email': "foo@example.org",
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
'fp_github_token': config['github']['token'],
|
|
|
|
'fp_github_name': 'herbert',
|
|
|
|
'branch_ids': [
|
|
|
|
(0, 0, {'name': 'a', 'sequence': 100}),
|
|
|
|
(0, 0, {'name': 'b', 'sequence': 80}),
|
|
|
|
],
|
|
|
|
"repo_ids": [
|
|
|
|
(0, 0, {
|
|
|
|
'name': prod.name,
|
|
|
|
'required_statuses': "default",
|
|
|
|
'fp_remote_target': prod.fork().name,
|
|
|
|
'group_id': False,
|
|
|
|
})
|
|
|
|
]
|
|
|
|
})
|
|
|
|
env['res.partner'].search([
|
|
|
|
('github_login', '=', config['role_reviewer']['user'])
|
|
|
|
]).write({
|
|
|
|
'review_rights': [(0, 0, {'repository_id': project.repo_ids.id, 'review': True})]
|
|
|
|
})
|
|
|
|
# endregion
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
prod.make_commits(
|
|
|
|
'a',
|
|
|
|
Commit("c", tree={
|
|
|
|
"foo/bar/baz": "2",
|
|
|
|
"corge/grault": "insert funny number",
|
|
|
|
}),
|
|
|
|
ref="heads/conflicting",
|
|
|
|
)
|
|
|
|
pr = prod.make_pr(target='a', head='conflicting')
|
|
|
|
prod.post_status("conflicting", "success")
|
|
|
|
pr.post_comment("hansen r+", config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
prod.post_status('staging.a', 'success')
|
|
|
|
env.run_crons()
|
|
|
|
_, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
assert prod.read_tree(prod.commit(pr1.head), recursive=True) == {
|
|
|
|
"foo/bar/qux": "2",
|
|
|
|
"foo/bar/baz": """\
|
|
|
|
<<<<<<< HEAD
|
|
|
|
||||||| MERGE BASE
|
|
|
|
=======
|
|
|
|
2
|
|
|
|
>>>>>>> FORWARD PORTED
|
|
|
|
""",
|
|
|
|
"corge/grault": """\
|
|
|
|
<<<<<<< HEAD
|
|
|
|
||||||| MERGE BASE
|
|
|
|
=======
|
|
|
|
insert funny number
|
|
|
|
>>>>>>> FORWARD PORTED
|
|
|
|
"""
|
|
|
|
}, "the conflicting file should have had conflict markers fixed in"
|
|
|
|
|
2021-08-02 14:18:30 +07:00
|
|
|
def test_multiple_commits_same_authorship(env, config, make_repo):
|
|
|
|
""" When a PR has multiple commits by the same author and its
|
|
|
|
forward-porting triggers a conflict, the resulting (squashed) conflict
|
|
|
|
commit should have the original author (same with the committer).
|
|
|
|
"""
|
|
|
|
author = {'name': 'George Pearce', 'email': 'gp@example.org'}
|
|
|
|
committer = {'name': 'G. P. W. Meredith', 'email': 'gpwm@example.org'}
|
|
|
|
prod, _ = make_basic(env, config, make_repo)
|
|
|
|
with prod:
|
|
|
|
# conflict: create `g` in `a`, using two commits
|
|
|
|
prod.make_commits(
|
|
|
|
'a',
|
|
|
|
Commit('c0', tree={'g': '1'},
|
|
|
|
author={**author, 'date': '1932-10-18T12:00:00Z'},
|
|
|
|
committer={**committer, 'date': '1932-11-02T12:00:00Z'}),
|
|
|
|
Commit('c1', tree={'g': '2'},
|
|
|
|
author={**author, 'date': '1932-11-12T12:00:00Z'},
|
|
|
|
committer={**committer, 'date': '1932-11-13T12:00:00Z'}),
|
|
|
|
ref='heads/conflicting'
|
|
|
|
)
|
|
|
|
pr = prod.make_pr(target='a', head='conflicting')
|
|
|
|
prod.post_status('conflicting', 'success', 'legal/cla')
|
|
|
|
prod.post_status('conflicting', 'success', 'ci/runbot')
|
|
|
|
pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
pr_id = to_pr(env, pr)
|
|
|
|
assert pr_id.state == 'ready'
|
|
|
|
assert pr_id.staging_id
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
prod.post_status('staging.a', 'success', 'legal/cla')
|
|
|
|
prod.post_status('staging.a', 'success', 'ci/runbot')
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
for _ in range(20):
|
|
|
|
pr_ids = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
if len(pr_ids) == 2:
|
|
|
|
_ , pr2_id = pr_ids
|
|
|
|
break
|
|
|
|
time.sleep(0.5)
|
|
|
|
else:
|
|
|
|
assert 0, "timed out"
|
|
|
|
|
|
|
|
c = prod.commit(pr2_id.head)
|
|
|
|
get = itemgetter('name', 'email')
|
|
|
|
assert get(c.author) == get(author)
|
|
|
|
assert get(c.committer) == get(committer)
|
|
|
|
|
2024-06-11 20:41:03 +07:00
|
|
|
|
2021-08-02 14:18:30 +07:00
|
|
|
def test_multiple_commits_different_authorship(env, config, make_repo, users, rolemap):
|
2021-08-03 18:45:21 +07:00
|
|
|
""" When a PR has multiple commits by different authors, the resulting
|
2022-10-28 13:03:12 +07:00
|
|
|
(squashed) conflict commit should have an empty email
|
2021-08-02 14:18:30 +07:00
|
|
|
"""
|
|
|
|
author = {'name': 'George Pearce', 'email': 'gp@example.org'}
|
|
|
|
committer = {'name': 'G. P. W. Meredith', 'email': 'gpwm@example.org'}
|
|
|
|
prod, _ = make_basic(env, config, make_repo)
|
|
|
|
with prod:
|
|
|
|
# conflict: create `g` in `a`, using two commits
|
|
|
|
# just swap author and committer in the commits
|
|
|
|
prod.make_commits(
|
|
|
|
'a',
|
|
|
|
Commit('c0', tree={'g': '1'},
|
|
|
|
author={**author, 'date': '1932-10-18T12:00:00Z'},
|
|
|
|
committer={**committer, 'date': '1932-11-02T12:00:00Z'}),
|
|
|
|
Commit('c1', tree={'g': '2'},
|
|
|
|
author={**committer, 'date': '1932-11-12T12:00:00Z'},
|
|
|
|
committer={**author, 'date': '1932-11-13T12:00:00Z'}),
|
|
|
|
ref='heads/conflicting'
|
|
|
|
)
|
|
|
|
pr = prod.make_pr(target='a', head='conflicting')
|
|
|
|
prod.post_status('conflicting', 'success', 'legal/cla')
|
|
|
|
prod.post_status('conflicting', 'success', 'ci/runbot')
|
|
|
|
pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
pr_id = to_pr(env, pr)
|
|
|
|
assert pr_id.state == 'ready'
|
|
|
|
assert pr_id.staging_id
|
|
|
|
|
|
|
|
with prod:
|
|
|
|
prod.post_status('staging.a', 'success', 'legal/cla')
|
|
|
|
prod.post_status('staging.a', 'success', 'ci/runbot')
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
for _ in range(20):
|
|
|
|
pr_ids = env['runbot_merge.pull_requests'].search([], order='number')
|
|
|
|
if len(pr_ids) == 2:
|
2022-10-28 13:03:12 +07:00
|
|
|
_, pr2_id = pr_ids
|
2021-08-02 14:18:30 +07:00
|
|
|
break
|
|
|
|
time.sleep(0.5)
|
|
|
|
else:
|
|
|
|
assert 0, "timed out"
|
|
|
|
|
|
|
|
c = prod.commit(pr2_id.head)
|
2021-08-03 18:45:21 +07:00
|
|
|
assert len(c.parents) == 1
|
2021-08-02 14:18:30 +07:00
|
|
|
get = itemgetter('name', 'email')
|
2023-06-12 19:41:42 +07:00
|
|
|
bot = pr_id.repository.project_id.fp_github_name
|
|
|
|
assert get(c.author) == (bot, ''), \
|
2021-08-03 18:45:21 +07:00
|
|
|
"In a multi-author PR, the squashed conflict commit should have the " \
|
|
|
|
"author set to the bot but an empty email"
|
2023-06-12 19:41:42 +07:00
|
|
|
assert get(c.committer) == (bot, '')
|
2021-08-03 18:45:21 +07:00
|
|
|
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
assert prod.read_tree(c)['g'] == matches('''<<<\x3c<<< b
|
2021-08-03 18:45:21 +07:00
|
|
|
b
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
||||||| $$
|
2021-08-03 18:45:21 +07:00
|
|
|
=======
|
|
|
|
2
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
>>>\x3e>>> $$
|
|
|
|
''')
|
2021-08-03 18:45:21 +07:00
|
|
|
|
|
|
|
# I'd like to fix the conflict so everything is clean and proper *but*
|
|
|
|
# github's API apparently rejects creating commits with an empty email.
|
|
|
|
#
|
|
|
|
# So fuck that, I'll just "merge the conflict". Still works at simulating
|
|
|
|
# a resolution error as technically that's the sort of things people do.
|
|
|
|
|
|
|
|
pr2 = prod.get_pr(pr2_id.number)
|
|
|
|
with prod:
|
|
|
|
prod.post_status(pr2_id.head, 'success', 'legal/cla')
|
|
|
|
prod.post_status(pr2_id.head, 'success', 'ci/runbot')
|
|
|
|
pr2.post_comment('hansen r+', config['role_reviewer']['token'])
|
|
|
|
env.run_crons()
|
|
|
|
|
|
|
|
assert pr2.comments == [
|
|
|
|
seen(env, pr2, users),
|
[IMP] forwardport: surfacing of modify/delete conflicts
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
2024-09-27 17:37:49 +07:00
|
|
|
(users['user'], matches('@%(user)s @%(reviewer)s $$CONFLICT' % users)),
|
2021-08-03 18:45:21 +07:00
|
|
|
(users['reviewer'], 'hansen r+'),
|
2022-06-23 19:25:07 +07:00
|
|
|
(users['user'], f"@{users['user']} @{users['reviewer']} unable to stage: "
|
|
|
|
"All commits must have author and committer email, "
|
2021-08-03 18:45:21 +07:00
|
|
|
f"missing email on {pr2_id.head} indicates the "
|
2022-06-23 19:25:07 +07:00
|
|
|
"authorship is most likely incorrect."),
|
2021-08-03 18:45:21 +07:00
|
|
|
]
|
|
|
|
assert pr2_id.state == 'error'
|
|
|
|
assert not pr2_id.staging_id, "staging should have been rejected"
|