diff --git a/contrib/review-prs.py b/contrib/review-prs.py index e1fd27aab5..f34ec9fd65 100755 --- a/contrib/review-prs.py +++ b/contrib/review-prs.py @@ -13,13 +13,35 @@ import shlex 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.""" @@ -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, @@ -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, @@ -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, @@ -140,7 +183,7 @@ def get_merged_commits(): "prno": prno, "trailing": trailing, "fromgui": fromgui, - "fromsecp": fromsecp + "fromsecp": fromsecp, }) return merged_commits @@ -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: @@ -189,7 +233,8 @@ 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) @@ -197,7 +242,7 @@ def main() -> None: 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}`).") @@ -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) @@ -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 = "" - 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") + 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: " + ("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 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 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. **)") + 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