Skip to content

Improvements to pr review script #1118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Improvements to pr review script #1118

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
229 changes: 162 additions & 67 deletions contrib/review-prs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,35 @@

from typing import List, Dict, Any

# XXX: Requires bitcoin, upstream, apolestra remotes to exist!!
BITCOIN_MASTER = "bitcoin/master"
ELEMENTS_MASTER = "upstream/master"
MERGED_MASTER = "apoelstra/merged-master"
MERGED_MASTER = "upstream/merged-master"
MERGED_MASTER_REVIEWED = "merged-master-reviewed"

# XXX: This is not actually a good idea. On many systems (incl. macOS) random
# files in the worktree will start to go missing a few days after its creation,
# because of temp-directory-cleaning scripts that work on a file-by-file basis.
WORKTREE_LOCATION = "/tmp/elements-merge-review-worktree"

def print_startup_warning() -> None:
print()
print("To prepare to use this script, make sure the following things are set up:")
print(" - The remotes 'bitcoin' and 'upstream' should point to Bitcoin Core and")
print(" Elements upstream repos, respectively.")
print(f" - The latest {MERGED_MASTER_REVIEWED} branch should be checked out locally.")
print(f" - We are currently using '{MERGED_MASTER}' as our branch to review.")
print(" To change this, edit the constant MERGED_MASTER in the script.")
print(" - We will reuse or create a git worktree at:")
print(f" {WORKTREE_LOCATION}")
print(" If this fails, delete that directory, run 'git worktree prune', and retry.")

print()
result = prompt_chars("Continue?", "yn")
if (result != "y"):
sys.exit(1)

ECHO_COMMANDS = True
GIT_DIFF_OPTS = "--color=always --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change --ignore-space-change --histogram"

class CommandFailed(Exception):
"""Exception for failure of a shell command."""
Expand Down Expand Up @@ -57,22 +79,24 @@ def prompt_chars(prompt: str, chars: str) -> str:
def get_bitcoin_commits():
"""Get the list of upstream bitcoin commits which should be included in merges not yet reviewed."""

BTC_COMMITS_CMD=f"git log {BITCOIN_MASTER} --not {MERGED_MASTER_REVIEWED} --merges --first-parent --reverse --pretty='format:%ct %cI %H Bitcoin %s'"
BTC_COMMITS_CMD=f"git log '{BITCOIN_MASTER}' --not '{MERGED_MASTER_REVIEWED}' --merges --first-parent --reverse --pretty='format:%ct %cI %H Bitcoin %s'"

bitcoin_commits_raw = shell(BTC_COMMITS_CMD)
bitcoin_commits = []
for line in bitcoin_commits_raw:
m = re.match(r"\d+ ([^ ]+) ([0-9a-f]+) Bitcoin Merge ((?:bitcoin-core/gui)?)#(\d+): (.*)", line)
m = re.match(r"\d+ ([^ ]+) ([0-9a-f]+) Bitcoin Merge ((?:[-_a-zA-Z0-9]*/[-_a-zA-Z0-9]*)?)#(\d+): (.*)", line)
if m is None:
raise Exception("Merge commit message in Bitcoin Core history had unexpected format")
[date, cid, guirepo, prno, msg] = m.groups()
raise Exception(f"Merge commit message in Bitcoin Core history had unexpected format:\n > {line}")
[date, cid, repo, prno, msg] = m.groups()
fromgui = None
if guirepo == "":
if repo == "":
fromgui = False
elif guirepo == "bitcoin-core/gui":
elif repo == "bitcoin/bitcoin":
fromgui = False
elif repo == "bitcoin-core/gui":
fromgui = True
else:
raise Exception("Merge into Bitcoin Core from unexpected repo, or commit message had unexpected format")
raise Exception(f"Merge into Bitcoin Core from unexpected repo, or commit message had unexpected format: \n > {line}")

bitcoin_commits.append({
"date": date,
Expand All @@ -86,15 +110,17 @@ def get_bitcoin_commits():
def get_elements_commits():
"""Get the list of elements commits which should be included in merges not yet reviewed."""

ELT_COMMITS_CMD=f"git log {ELEMENTS_MASTER} --not {MERGED_MASTER_REVIEWED} --merges --first-parent --reverse --pretty='format:%ct %cI %H Elements %s'"
ELT_COMMITS_CMD=f"git log '{ELEMENTS_MASTER}' --not '{MERGED_MASTER_REVIEWED}' --merges --first-parent --reverse --pretty='format:%ct %cI %H Elements %s'"

elements_commits_raw = shell(ELT_COMMITS_CMD)
elements_commits = []
for line in elements_commits_raw:
m = re.fullmatch(r"\d+ ([^ ]+) ([0-9a-f]+) Elements Merge (?:pull request )?#(\d+):? (.*)", line)
m = re.fullmatch(r"\d+ ([^ ]+) ([0-9a-f]+) Elements Merge (?:pull request )?((?:[-_a-zA-Z0-9]*/[-_a-zA-Z0-9]*)?)#(\d+):? (.*)", line)
if m is None:
raise Exception("Merge commit message in Elements history had unexpected format")
[date, cid, prno, msg] = m.groups()
raise Exception(f"Merge commit message in Elements history had unexpected format: \n > {line}")
[date, cid, repo, prno, msg] = m.groups()
if repo != "" and repo != "ElementsProject/elements":
raise Exception(f"Merge into Elements from unexpected repo, or commit message had unexpected format: \n > {line}")
elements_commits.append({
"date": date,
"cid": cid,
Expand All @@ -106,31 +132,48 @@ def get_elements_commits():
def get_merged_commits():
"""Get the list of merge commits still needing to be reviewed."""

MERGED_COMMITS_CMD=f"git log {MERGED_MASTER} --not {MERGED_MASTER_REVIEWED} --first-parent --reverse --pretty='format:%ct %cI %H Merged %s'"
MERGED_COMMITS_CMD=f"git log '{MERGED_MASTER}' --not '{MERGED_MASTER_REVIEWED}' --first-parent --reverse --pretty='format:%ct %cI %H Merged %s'"

merged_commits_raw = shell(MERGED_COMMITS_CMD)
merged_commits = []
for line in merged_commits_raw:
#print(line)
m = re.fullmatch(r"\d+ ([^ ]+) ([0-9a-f]+) Merged Merge ([0-9a-f]+) into merged_master \((.+) PR ((?:bitcoin-core/gui)?)#(\d+)\)(.*)", line)
m = re.fullmatch(r"\d+ ([^ ]+) ([0-9a-f]+) Merged Merge ([0-9a-f]+) into merged_master \((.+) PR (?:pull )?((?:[-_a-zA-Z0-9]*/[-_a-zA-Z0-9]*)?)#(\d+)\)(.*)", line)
m_secp = re.fullmatch(r"\d+ ([^ ]+) ([0-9a-f]+) Merged update libsecp256k1-zkp to ([0-9a-f]+)", line)
m_misc = re.fullmatch(r"\d+ ([^ ]+) ([0-9a-f]+) Merged (.*)$", line)

fromgui = False
fromsecp = False

if m is not None:
[date, cid, merged_cid, chain, guirepo, prno, trailing] = m.groups()
if guirepo == "bitcoin-core/gui":
fromgui = True
elif guirepo != "":
raise Exception("Merge into Bitcoin Core from unexpected repo, or commit message had unexpected format")
[date, cid, merged_cid, chain, repo, prno, trailing] = m.groups()
if chain == "Bitcoin":
if repo == "bitcoin-core/gui":
fromgui = True
elif repo != "" and repo != "bitcoin/bitcoin":
raise Exception(f"Merge into Bitcoin Core from unexpected repo, or commit message had unexpected format:\n > {line}")
elif chain == "Elements":
if repo != "" and repo != "ElementsProject/elements":
raise Exception(f"Merge into Elements from unexpected repo, or commit message had unexpected format:\n > {line}")
else:
raise Exception(f"Commit message had unexpected format:\n > {line}")
elif m_secp is not None:
[date, cid, merged_cid] = m_secp.groups()
fromsecp = True
chain = ""
prno = ""
trailing = ""
elif m_misc is not None:
[date, cid, trailing] = m_misc.groups()
merged_commits.append({
"date": date,
"cid": cid,
"trailing": trailing,
"handreview": True,
})
continue
else:
raise Exception(f"Commit {cid} did not match any of the expected templates. Please hand-review.")
raise Exception(f"Commit {cid} did not match any of the expected templates. Not sure what to do.\n > {line}")

merged_commits.append({
"date": date,
Expand All @@ -140,7 +183,7 @@ def get_merged_commits():
"prno": prno,
"trailing": trailing,
"fromgui": fromgui,
"fromsecp": fromsecp
"fromsecp": fromsecp,
})
return merged_commits

Expand All @@ -150,7 +193,8 @@ def check_commit(commit, last_reviewed, incoming_commit) -> None:
cid = commit["cid"]
parents = shell(f"git cat-file -p {cid} | grep '^parent'")
if len(parents) != 2:
raise Exception(f"Expected merge, but commit {cid} has {parents} parents, which is not two! Please hand-review.")
print(f"WARNING: Expected merge, but commit {cid} has {parents} parents, which is not two! Please hand-review.")
return

parent_cids = []
for p in parents:
Expand Down Expand Up @@ -189,15 +233,16 @@ def check_or_create_worktree(path) -> None:
def main() -> None:
"""Interactively review commits."""

WORKTREE_LOCATION = "/tmp/elements-merge-review-worktree"
print_startup_warning()

check_or_create_worktree(WORKTREE_LOCATION)
WQ = shlex.quote(WORKTREE_LOCATION)

print("Fetching all remotes...")
os.system(f"git -C {WQ} fetch --all")

print()
print(f"For our already-reviewed base branch, using `{MERGED_MASTER_REVIEWED}`. Starting from there:")
print(f"For our already-reviewed base branch, using `{MERGED_MASTER_REVIEWED}`. If this fails, create / check out that branch locally from upstream's copy. Starting from there:")

bitcoin_commits = get_bitcoin_commits()
print(f"Found a total of {len(bitcoin_commits)} Bitcoin Core commits to review (on `{BITCOIN_MASTER}`).")
Expand Down Expand Up @@ -227,24 +272,27 @@ def main() -> None:
sys.exit(1)
print()

SKIP_CLEAN = False
SKIP_CLEAN = True

for commit in merged_commits:
REQUIRE_MANUAL_REVIEW = False
# Figure out what incoming/upstream commit this is merging, from which side.
incoming_commit: Dict[str, Any] = {}
if commit["chain"] == "Bitcoin":
if commit.get("chain", None) == "Bitcoin":
incoming_commit = bitcoin_commits.pop(0)
elif commit["chain"] == "Elements":
elif commit.get("chain", None) == "Elements":
incoming_commit = elements_commits.pop(0)
elif commit["fromsecp"]:
elif commit.get("fromsecp", False):
incoming_commit = {"cid": commit["merged_cid"]}
print()
print("WARNING: The next commit claims to merge a libsecp256k1 commit. We have no way to verify where it came from.")
print()
REQUIRE_MANUAL_REVIEW = True
else:
raise Exception(f"This is neither a Bitcoin nor an Elements merge; I don't know what to do with it. Details: {commit}")
# TODO: A bunch of stuff doesn't really work right in this mode. Fix the UI so it makes sense.
incoming_commit = None
print(f"WARNING: This is neither a Bitcoin nor an Elements merge; I don't know what to do with it. Details: {commit}")
REQUIRE_MANUAL_REVIEW = True

check_commit(commit, last_reviewed, incoming_commit)

Expand All @@ -255,57 +303,104 @@ def main() -> None:
# stderr is going to yell at us about throwing away our temp commit, so ignore it
shell(f"git -C {WQ} checkout {last_reviewed}", suppress_stderr=True)

# We expect this to complain if the merge is not clean, but that's okay, so we ignore it.
shell(f"git -C {WQ} merge --no-rerere-autoupdate --no-ff --no-commit {incoming_commit['cid']}", suppress_stderr=True, check_returncode=False)
if incoming_commit is not None:
# We expect this to complain if the merge is not clean, but that's okay, so we ignore it.
shell(f"git -C {WQ} merge --no-rerere-autoupdate --no-ff --no-commit {incoming_commit['cid']}", suppress_stderr=True, check_returncode=False)

shell(f"git -C {WQ} add -u")
shell(f"git -C {WQ} commit -m 'TEMP REVIEW COMMIT'")

shell(f"git -C {WQ} add -u")
shell(f"git -C {WQ} commit -m 'TEMP REVIEW COMMIT'")
diff = shell(f"git -C {WQ} diff --histogram HEAD {cid}")
diffstr = "\n".join(diff)
else:
diffstr = "<Could not find incoming commit; no diff available>"

diff = shell(f"git -C {WQ} diff --histogram HEAD {cid}")
diffstr = "\n".join(diff)
if len(diffstr) == 0 and SKIP_CLEAN and (not REQUIRE_MANUAL_REVIEW):
last_reviewed = cid
continue

# Force 'less' to always wait before exiting, so that short output doesn't get lost when followed by long output.
os.environ["LESS"] = "RSX"
said = ""
while said != "y":
print()
print("** COMMIT WE ARE REVIEWING:")
os.system(f"git -C {WQ} log -1 --pretty=full {cid}")
print()

print()
print("** COMMIT WE ARE REVIEWING:")
os.system(f"git -C {WQ} log -1 --pretty=full {cid}")
print()

def diff4():
os.system(f"git -C {WQ} diff {GIT_DIFF_OPTS} HEAD {cid}")

if incoming_commit is not None:
print("** UPSTREAM COMMIT MERGED THEREIN:")
os.system(f"git -C {WQ} log -1 --pretty=full {incoming_commit['cid']}")
print()

if len(diffstr) > 0:
print("** AND HERE'S THE DIFF:")
print()
os.system(f"git -C {WQ} diff --color=always --color-moved=dimmed_zebra --ignore-space-change --histogram HEAD {cid}")
else:
print("** No diff, merge was clean!")
diff4()

print()
said = prompt_chars("Accept this commit? (If unsure, say 'N', which will exit the review script.) Or [r]edisplay commit, or [a]utoskip clean diffs?", "ynra")

if said == "a":
SKIP_CLEAN = not SKIP_CLEAN
print("Autoskip clean diffs: " + ("on" if SKIP_CLEAN else "off"))
said = "y"
continue
elif said == "r":
continue
elif said == "n":
print(f"The current commit (in `{WORKTREE_LOCATION}`) is the conflicted diff as git produced it. The command we used to diff it against the actual merge was:")
print(f"git -C {WQ} diff --color=always --color=moved=dimmed_zebra --ignore-space-change --histogram --exit-code HEAD {cid}")
print("Look over the commits however you like, or contact someone for assistance. If you are satisfied that the diff is okay, run the review script again.")
sys.exit(1)

if prompt_chars("Locally mark all commits up to and including this commit as reviewed, so we skip them in the future?", "yn") == "y":
shell(f"git -C {WQ} branch -f {MERGED_MASTER_REVIEWED} {cid}")
print(f"Done. (Updated local branch `{MERGED_MASTER_REVIEWED}` in `{WORKTREE_LOCATION}`. ** You will need to push this branch to gitlab if you want to persist this. **)")
said = ""
if incoming_commit is not None:
while said != "y":
said = prompt_chars("Accept this commit? (If unsure, say 'N', which will exit the review script.) Or show a [d]iff, [r]edisplay commit, or [a]utoskip clean diffs [toggle]?", "yndra")

if said == "a":
SKIP_CLEAN = not SKIP_CLEAN
print("Autoskip clean diffs now: " + ("on" if SKIP_CLEAN else "off"))
said = "y"
continue
elif said == "r":
print()
print("** COMMIT WE ARE REVIEWING:")
os.system(f"git -C {WQ} log -1 --pretty=full {cid}")
print()

if incoming_commit is not None:
print("** UPSTREAM COMMIT MERGED THEREIN:")
os.system(f"git -C {WQ} log -1 --pretty=full {incoming_commit['cid']}")
print()

elif said == "d":
diff = prompt_chars("Type of diff to show: [4]way, [o]rig-commits, [m]erged-commits, merged-[f]lattened, or get [h]elp?", "4omfh")
if diff == "4":
diff4()
elif diff == "o":
os.system(f"git -C {WQ} show {GIT_DIFF_OPTS} {commit['merged_cid']}^1..{commit['merged_cid']}")
elif diff == "m":
os.system(f"git -C {WQ} show {GIT_DIFF_OPTS} {cid}^1..{cid}")
elif diff == "f":
os.system(f"git -C {WQ} diff {GIT_DIFF_OPTS} {cid}^1..{cid}")
elif diff == "h":
print("Diff modes supported:")
print(" - 4way: shows a very confusing diff between (1) the conflicted merge git would have made, left to its own devices, and (2) the resolved merge in the actual merged history.")
print(" - orig-commits: shows the commits from the original (upstream) PR, using `git show`.")
print(" - merged-commits: shows the commits of the PR as merged into the actual merged history, using `git show`. This will normally be the same commits as upstream, plus a merge commit resolving the merge conflicts, which is shown in a confusing format as described under `COMBINED DIFF FORMAT` in `man git-diff`.")
print(" - merged-flattened: shows a single flattened diff of the entire PR as applied in the actual merged history.")
# XXX: It would be nice to have a mode showing merged-flattened and orig-flattened (which I maybe could add) side-by-side.
print()

elif said == "n":
break
else: # incoming_commit is None
os.system(f"git -C {WQ} show {GIT_DIFF_OPTS} {cid}")
while said != "y":
said = prompt_chars("Accept this commit? (If unsure, say 'N', which will exit the review script.) Or [r]edisplay commit and diff?", "ynr")

if said == "r":
print()
print("** COMMIT WE ARE REVIEWING:")
os.system(f"git -C {WQ} log -1 --pretty=full {cid}")
print()
os.system(f"git -C {WQ} show {GIT_DIFF_OPTS} {cid}")
elif said == "n":
break

if said == "n":
print(f"The current commit (in `{WORKTREE_LOCATION}`) is the conflicted diff as git produced it. The command we used to diff it against the actual merge was:")
print(f"git -C {WQ} diff {GIT_DIFF_OPTS} HEAD {cid}")
print("Look over the commits however you like, or contact someone for assistance. If you are satisfied that the diff is okay, run the review script again.")
sys.exit(1)

shell(f"git -C {WQ} branch -f {MERGED_MASTER_REVIEWED} {cid}")
print(f"Done. (Updated local branch `{MERGED_MASTER_REVIEWED}` in `{WORKTREE_LOCATION}`. ** You will need to push this branch to gitlab if you want to persist this. **)")
print()

last_reviewed = cid
Expand Down