mirror of
https://github.com/ElementsProject/elements.git
synced 2026-08-15 12:51:00 +02:00
Merge pull request #1118 from gwillen/feature-merge-workflow
Improvements to pr review script
This commit is contained in:
Glenn Willen
GitHub
commit
718ac5bd6b
1 changed files with 160 additions and 65 deletions
|
|
@ -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 = "<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")
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue