Skip to content

Commit 6400839

Browse files
committed
Add the resolve command to finish the conflicted backports
Walks the branches whose cherry-pick stopped, one at a time, naming the worktree and the conflicting files, then finishes the pick and offers to open the pull requests. Where CI did the analysis there is no saved run on the reviewer's machine, so --pr takes the branch list from the report the bot left on the pull request Two independent refusals before a pick is finished, and both are needed. git's own unmerged list catches a conflict nobody touched. The marker scan catches a file staged with the markers still in it, which git is perfectly happy to commit. Neither check finds what the other does: a delete or rename conflict carries no markers at all, and staging a file is exactly what clears it out of the unmerged list Nothing is staged on the user's behalf. git add is how they say which side they chose, and an earlier version of this ran git add -A before continuing, which took an arbitrary side of a delete conflict and dropped half the fix while looking clean publish now folds the run into its report on the source pull request as JSON, which is what resolve reads back
1 parent 8f25b64 commit 6400839

7 files changed

Lines changed: 412 additions & 6 deletions

File tree

util/backport/README.md

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ passes:
1717
review, but a no-answer always leaves the branch flagged, so it can never hide a
1818
needed backport.
1919

20-
`apply` then cherry-picks the fix onto one local branch per affected branch, and
21-
`publish` turns those into one pull request each.
20+
`apply` then cherry-picks the fix onto one local branch per affected branch, `resolve`
21+
helps you finish any that conflicted, and `publish` turns them into one pull request
22+
each.
2223

2324
Nothing is auto-merged, and nothing is ever a draft. Every pull request needs review.
2425

@@ -240,6 +241,42 @@ Nothing is ever a draft and nothing is auto-merged. Re-running is safe: a branch
240241
already has a pull request is left alone, and finishing a conflict by hand is enough
241242
to let the next run pick it up, with no need to run `apply` again.
242243

244+
### Finish the conflicted ones
245+
246+
```bash
247+
util/backport/backport resolve
248+
```
249+
250+
Walks the branches whose cherry-pick stopped, one at a time. For each it names the
251+
worktree and the conflicting files, waits while you fix them, then finishes the pick
252+
and offers to open the pull requests.
253+
254+
When the bot did the analysis you have no saved run, so point it at the pull request
255+
the bot reported on and it takes the branch list from there:
256+
257+
```bash
258+
util/backport/backport resolve --pr 3401
259+
```
260+
261+
| Flag | Purpose |
262+
| --- | --- |
263+
| `--branch` | just this release branch |
264+
| `--pr` | take the branch list from the bot's report on that pull request |
265+
| `--remote` | fork remote the branches are pushed to, `origin` by default |
266+
267+
Resolve the conflict the way you would any other: edit the files in the worktree and
268+
`git add` each one. `resolve` will not finish the pick until git reports nothing
269+
unmerged **and** no staged file still contains a conflict marker. Both checks matter,
270+
for different reasons:
271+
272+
- A delete or rename conflict has no markers at all, so a marker scan alone would wave
273+
it through and silently keep one side.
274+
- Staging a file is what clears it from git's unmerged list, so once you have staged
275+
something badly, only the marker scan can still see it.
276+
277+
It never stages anything for you. `git add` is how you say which side you chose, and
278+
guessing on your behalf is how a backport quietly loses half of a fix.
279+
243280
## Running In CI
244281

245282
`.github/workflows/backport-bot.yml` does the same three steps automatically. It fires
@@ -256,6 +293,19 @@ It runs as two jobs, and the split is the point:
256293
The model reads repository content, so the job that reads it is never the job holding a
257294
token that could change it. The verdict travels between them as an artifact.
258295

296+
The bot only opens the pull requests it can cherry-pick cleanly. A branch that
297+
conflicts is reported in its comment on the source pull request and left alone, because
298+
nobody is there to choose a side. That comment carries the run as JSON, so the reviewer
299+
picks it up from there:
300+
301+
```bash
302+
util/backport/backport apply
303+
util/backport/backport resolve --pr 3401
304+
```
305+
306+
`apply` recreates the branches locally, `resolve` walks the conflicts, and it offers to
307+
open the remaining pull requests when they are finished.
308+
259309
Branches are pushed to `aws/aws-lc` itself, because in CI the checkout already is
260310
`aws/aws-lc`. That needs `--push-to-aws-lc`, which is refused everywhere else,
261311
and `push_branch` will only ever push a `backport-` branch, so the escape cannot reach
@@ -351,7 +401,8 @@ util/backport/
351401
│ ├── commands/
352402
│ │ ├── analyze.py # the analyze command
353403
│ │ ├── apply.py # the apply command
354-
│ │ └── publish.py # the publish command
404+
│ │ ├── publish.py # the publish command
405+
│ │ └── resolve.py # the resolve command
355406
│ ├── engine/
356407
│ │ ├── inspect_fix.py # which lines the fix deletes, who wrote them
357408
│ │ ├── discover_branches.py # which release branches to check

util/backport/src/commands/publish.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
create_pr,
2424
existing_pr,
2525
head_spec,
26+
plan_block,
2627
pr_title_and_body,
2728
push_branch,
2829
require_gh,
@@ -165,7 +166,8 @@ def report(
165166
print(" Failed branches: see the reason above.")
166167
if source_pr:
167168
table = summary_lines(run["fix"], commit_subject(run["fix"]), outcomes)
168-
failure = comment_on_pr(repo, source_pr, table)
169+
body = table + "\n\n" + plan_block(run["fix"], outcomes)
170+
failure = comment_on_pr(repo, source_pr, body)
169171
if failure:
170172
print(f"could not comment on #{source_pr}: {failure}")
171173

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
# SPDX-License-Identifier: Apache-2.0 OR ISC
3+
4+
"""
5+
The resolve command: walks the conflicts apply left behind, one branch at a time
6+
Takes its list of branches from the last local run, or from the report the bot left
7+
on a pull request, which is the only thing a reviewer has when CI did the analysis
8+
"""
9+
10+
from commands.apply import backport_branch_name, branches_to_backport, load_run
11+
from commands.publish import branch_state, offer_publish, UNFINISHED
12+
from util.config import BackportError
13+
from util.git import (
14+
WORKTREE_ROOT,
15+
continue_cherry_pick,
16+
files_with_conflict_markers,
17+
staged_files,
18+
unmerged_files,
19+
)
20+
from util.github import base_repo, read_plan
21+
from util.render import ask_yn
22+
23+
from typing import Dict, Tuple
24+
25+
# What happened to one branch
26+
FINISHED = "finished"
27+
LEFT = "left alone"
28+
STILL_STUCK = "still stuck"
29+
NOTHING_TO_DO = "nothing to do"
30+
31+
32+
def run_from_pr(number: str) -> Dict:
33+
"""
34+
The run the bot recorded on a pull request, as a run-shaped dict
35+
Returns {"fix": sha, "verdicts": {...}}. Raises when the pull request carries no
36+
plan, since guessing which branches were flagged would be worse than stopping
37+
"""
38+
plan = read_plan(base_repo("upstream"), number)
39+
if plan is None:
40+
raise BackportError(
41+
f"no backport plan found on #{number}.\n"
42+
" Only a pull request the bot has reported on carries one."
43+
)
44+
return {
45+
"fix": plan["fix"],
46+
# Every branch the bot touched is worth offering, and branch_state below is
47+
# what decides which of them actually still need work
48+
"verdicts": {b: "affected" for b in plan.get("branches", {})},
49+
}
50+
51+
52+
def resolve_branch(release: str, local_branch: str) -> Tuple[str, str]:
53+
"""
54+
Walks one conflicted branch: shows the files, waits, then finishes the pick
55+
Returns (outcome, detail)
56+
"""
57+
worktree = WORKTREE_ROOT / local_branch
58+
stuck = unmerged_files(worktree)
59+
print()
60+
print(f"{release}")
61+
print(f" worktree: {worktree}")
62+
if stuck:
63+
print(f" {len(stuck)} file(s) still conflicting:")
64+
for name in stuck:
65+
print(f" {name}")
66+
print(" Fix them there and 'git add' each one. Nothing is committed until you")
67+
print(" say so, and the pick is only finished once git sees nothing unmerged.")
68+
69+
while True:
70+
if not ask_yn(" Resolved?"):
71+
return LEFT, "come back to it with 'backport resolve'"
72+
# git's own view first. A delete or rename conflict carries no markers, so a
73+
# marker check alone would wave it through and quietly keep one side
74+
still = unmerged_files(worktree)
75+
if still:
76+
print(f" git still calls {len(still)} file(s) unmerged:")
77+
for name in still:
78+
print(f" {name}")
79+
print(" Choose a side, then 'git add' each of them.")
80+
continue
81+
markers = files_with_conflict_markers(
82+
worktree, sorted(set(stuck) | set(staged_files(worktree)))
83+
)
84+
if markers:
85+
# git will happily commit a file with the markers still in it
86+
print(f" These still have conflict markers: {', '.join(markers)}")
87+
continue
88+
failure = continue_cherry_pick(worktree)
89+
if failure:
90+
print(f" git could not finish it: {failure}")
91+
continue
92+
return FINISHED, f"{local_branch} is ready"
93+
94+
95+
def cmd_resolve(args) -> int:
96+
"""
97+
Finishes the conflicted backports, then offers to open their pull requests
98+
Returns 0 when nothing is left conflicting, 1 when a branch was left alone
99+
"""
100+
run = run_from_pr(args.pr) if args.pr else load_run()
101+
branches = branches_to_backport(run["verdicts"], args.branch)
102+
fix = run["fix"]
103+
104+
stuck = [
105+
b
106+
for b in branches
107+
if branch_state(b, backport_branch_name(fix, b)) == UNFINISHED
108+
]
109+
if not stuck:
110+
print("Nothing is waiting on a conflict.")
111+
if args.pr:
112+
print("Run 'backport apply' first if the branches are not here yet.")
113+
return 0
114+
115+
print(f"Fix {fix[:10]}, {len(stuck)} branch(es) to resolve: {', '.join(stuck)}")
116+
outcomes = {}
117+
for release in stuck:
118+
outcomes[release] = resolve_branch(release, backport_branch_name(fix, release))
119+
120+
finished = [b for b, (o, _) in outcomes.items() if o == FINISHED]
121+
left = [b for b, (o, _) in outcomes.items() if o != FINISHED]
122+
print()
123+
print(f"{len(finished)} of {len(stuck)} resolved")
124+
if left:
125+
print(f"Still conflicting: {', '.join(left)}")
126+
127+
if finished:
128+
offer_publish(run, finished, args.remote)
129+
return 1 if left else 0

util/backport/src/main.py

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from commands.analyze import cmd_analyze
1010
from commands.apply import cmd_apply
1111
from commands.publish import cmd_publish
12+
from commands.resolve import cmd_resolve
1213
from util.config import BackportError
1314

1415
import argparse
@@ -97,10 +98,32 @@ def add_publish(subparsers) -> None:
9798
p.set_defaults(func=cmd_publish)
9899

99100

101+
def add_resolve(subparsers) -> None:
102+
"""
103+
Walks the conflicts apply left behind, one branch at a time
104+
Takes the branch list from the last local run, or from --pr when the bot did it
105+
"""
106+
p = subparsers.add_parser(
107+
"resolve", help="finishes the conflicted backports, one branch at a time"
108+
)
109+
p.add_argument("--branch", help="only this release branch")
110+
p.add_argument(
111+
"--pr",
112+
help="take the branch list from the bot's report on this pull request, "
113+
"for when CI did the analysis",
114+
)
115+
p.add_argument(
116+
"--remote",
117+
default="origin",
118+
help="fork remote the branches are pushed to (default origin)",
119+
)
120+
p.set_defaults(func=cmd_resolve)
121+
122+
100123
def build_parser() -> argparse.ArgumentParser:
101124
"""
102125
Build parser for args
103-
Returns the parser, with the analyze, apply and publish subcommands on it
126+
Returns the parser, with the analyze, apply, publish and resolve subcommands on it
104127
"""
105128
ap = argparse.ArgumentParser(
106129
prog="backport",
@@ -110,6 +133,7 @@ def build_parser() -> argparse.ArgumentParser:
110133
add_analyze(subparsers)
111134
add_apply(subparsers)
112135
add_publish(subparsers)
136+
add_resolve(subparsers)
113137
return ap
114138

115139

util/backport/src/util/git.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import subprocess
2020
from contextlib import contextmanager
2121
from functools import lru_cache
22+
from pathlib import Path
2223
from typing import Dict, Iterator, List, Optional, Sequence, Tuple
2324

2425
# --- Where We Run ---
@@ -469,3 +470,66 @@ def commit_subject(commit: str) -> str:
469470
"""The one-line subject of a commit, or an empty string when git cannot read it"""
470471
subject = git("log", "-1", "--format=%s", commit, check=False)
471472
return subject.stdout.strip() if subject.returncode == 0 else ""
473+
474+
475+
def unmerged_files(path) -> List[str]:
476+
"""The still-conflicting files in a worktree. Empty when nothing is unmerged"""
477+
listed = git("-C", str(path), "diff", "--name-only", "--diff-filter=U", check=False)
478+
if listed.returncode != 0:
479+
return []
480+
return [f for f in listed.stdout.splitlines() if f.strip()]
481+
482+
483+
def staged_files(path) -> List[str]:
484+
"""
485+
The files a stopped cherry-pick is about to commit in that worktree
486+
Staging is what clears a file out of the unmerged list, so this is the only view
487+
that still sees a file the user staged with the markers left in
488+
"""
489+
listed = git("-C", str(path), "diff", "--cached", "--name-only", check=False)
490+
if listed.returncode != 0:
491+
return []
492+
return [f for f in listed.stdout.splitlines() if f.strip()]
493+
494+
495+
def files_with_conflict_markers(path, files: Sequence[str]) -> List[str]:
496+
"""
497+
Which of those files still carry a merge marker
498+
Checked before finishing a cherry-pick, because git is happy to commit a file with
499+
the markers left in and the result compiles as nonsense
500+
"""
501+
left = []
502+
for name in files:
503+
try:
504+
body = (Path(str(path)) / name).read_text(
505+
encoding="utf-8", errors="replace"
506+
)
507+
except OSError:
508+
continue # deleted as part of the resolution, nothing to check
509+
for line in body.splitlines():
510+
if line.startswith(("<<<<<<<", ">>>>>>>")):
511+
left.append(name)
512+
break
513+
return left
514+
515+
516+
def continue_cherry_pick(path) -> Optional[str]:
517+
"""
518+
Finishes a cherry-pick whose conflicts have already been staged
519+
Returns None when it completed, or git's complaint. Deliberately does not stage
520+
anything: 'git add' is how the user says which side of a conflict they chose, and
521+
a delete or rename conflict has no markers to check, so staging on their behalf
522+
would silently pick one for them
523+
"""
524+
done = git(
525+
"-C",
526+
str(path),
527+
"-c",
528+
"core.editor=true",
529+
"cherry-pick",
530+
"--continue",
531+
check=False,
532+
)
533+
if done.returncode != 0:
534+
return (done.stderr or done.stdout).strip()
535+
return None

0 commit comments

Comments
 (0)