Skip to content

WIP: Push multiple branches in the stack atomically: it's faster, plus possibly improves CODEOWNERS processing by GitHub #26

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: Push multiple branches in the stack atomically: it's faster, plus possibly improves CODEOWNERS processing by GitHub #26

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
157 changes: 107 additions & 50 deletions git-grok
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ class Pr:
labels: list[str]


#
# A pair of commit hash and branch name.
#
@dataclass
class Branch:
hash: str
branch: str
push_result: BranchPushResult | None = None


#
# Some data passed from the main process to each individual child process call
# within "git rebase -i", for each commit in the stack.
Expand Down Expand Up @@ -127,6 +137,7 @@ class Main:
login: str
remote: str
remote_base_branch: str = "master"
in_rebase_interactive: InRebaseInteractiveData | None = None

#
# Main entry point.
Expand All @@ -151,14 +162,14 @@ class Main:
)
args = parser.parse_args()

in_rebase_interactive = InRebaseInteractiveData.parse(
self.in_rebase_interactive = InRebaseInteractiveData.parse(
os.environ.get(INTERNAL_IN_REBASE_INTERACTIVE_VAR, "")
)

self.debug = args.debug
self.debug_force_push_branches = args.debug_force_push_branches

if not in_rebase_interactive:
if not self.in_rebase_interactive:
self.gh_verify_version()
self.git_verify_version()
self.self_update()
Expand All @@ -179,10 +190,10 @@ class Main:
"git_get_current_remote_base_branch",
self.git_get_current_remote_base_branch,
)
if not in_rebase_interactive:
if not self.in_rebase_interactive:
self.run_all()
else:
self.run_in_rebase_interactive(data=in_rebase_interactive)
self.run_in_rebase_interactive(data=self.in_rebase_interactive)

#
# Assuming all PRs in the stack already have PR URLs in their description,
Expand Down Expand Up @@ -227,6 +238,8 @@ class Main:
commit_with_no_url = commit
break
else:
# Push this branch and see whether GitHub says whether it
# was up to date or not.
commit_hashes_to_push_branch.append(commit.hash)

# Some commits have no related PRs (no GitHub URLs in the message)?
Expand Down Expand Up @@ -270,23 +283,36 @@ class Main:

self.print_header(f"Processing commit: {self.clean_title(commit.title)}")

to_push: list[Branch] = []

if prev_commit.hash != remote_commit.hash:
prev_commit, result = self.process_commit_push_branch(commit=prev_commit)
prev_commit.branch = self.process_commit_infer_branch(commit=prev_commit)
to_push.append(Branch(hash=prev_commit.hash, branch=prev_commit.branch))
else:
prev_commit.branch = None

commit.branch = self.process_commit_infer_branch(commit=commit)
to_push.append(Branch(hash=commit.hash, branch=commit.branch))

push_results = self.git_push_branches(branches=to_push)

if prev_commit.branch:
self.print_branch_result(
type="base",
branch=str(prev_commit.branch),
result=result,
branch=prev_commit.branch,
result=push_results[prev_commit.branch],
)
else:
self.print_branch_result(
type="base",
branch=self.remote_base_branch,
result="up-to-date",
)
prev_commit.branch = None

commit, result = self.process_commit_push_branch(commit=commit)
self.print_branch_result(type="head", branch=str(commit.branch), result=result)
self.print_branch_result(
type="head",
branch=commit.branch,
result=push_results[commit.branch],
)

new_pr_title = commit.title
new_pr_body = None
Expand Down Expand Up @@ -342,11 +368,11 @@ class Main:
commits: list[Commit],
prs_by_url: dict[str, Pr],
):
commits_chronological = list(reversed(commits))
for i, commit in list(enumerate(commits_chronological))[1:]:
commits_old_to_new = list(reversed(commits))
for i, commit in list(enumerate(commits_old_to_new))[1:]:
pr = prs_by_url.get(commit.url, None) if commit.url else None
if pr and pr.auto_merge_status == "ENABLED":
prev_commit = commits_chronological[i - 1]
prev_commit = commits_old_to_new[i - 1]
prev_pr = (
prs_by_url.get(prev_commit.url, None) if prev_commit.url else None
)
Expand Down Expand Up @@ -426,25 +452,35 @@ class Main:
):
# We must iterate from the oldest commit to the newest one, because
# previous commit PR's branch becomes the next commit PR's base branch.
commits_chronological = list(reversed(commits))
for i, commit in enumerate(commits_chronological):
commits_old_to_new = list(reversed(commits))

# Push all branches atomically, in bulk. This is meant to prevent
# useless CODEOWNERS reviewers addition in case the commits were
# reordered or rebased, and is faster in general.
to_push: list[Branch] = []
for commit in commits_old_to_new:
if (
self.debug_force_push_branches
or commit.hash in commit_hashes_to_push_branch
):
commit.branch = self.process_commit_infer_branch(commit=commit)
to_push.append(Branch(hash=commit.hash, branch=commit.branch))
push_results = self.git_push_branches(branches=to_push)

for i, commit in enumerate(commits_old_to_new):
self.print_header(f"Updating PR: {self.clean_title(commit.title)}")

if commit.hash in commit_hashes_to_push_branch:
commit, result = self.process_commit_push_branch(commit=commit)
if commit.branch in push_results:
result = push_results[commit.branch]
if result == "pushed":
self.print_branch_result(
type="head",
branch=str(commit.branch),
branch=commit.branch,
result=result,
)
commits_chronological[i] = commit

assert (
commit.url is not None
), f"commit {commit.hash} PR url is expected to be in the message at this point"
pr, result = self.process_update_pr(
prev_commit=commits_chronological[i - 1] if i > 0 else None,
prev_commit=commits_old_to_new[i - 1] if i > 0 else None,
commit=commit,
commits=commits,
)
Expand All @@ -453,27 +489,26 @@ class Main:
result=result,
review_decision=pr.review_decision,
)
commits_chronological[i].branch = pr.head_branch
commits_old_to_new[i].branch = pr.head_branch

#
# Pushes an existing branch (it we know this commit's PR URL by querying
# GitHub), or creates a new branch based on commit title and pushes it.
# For a commit, infers its corresponding remote branch name by either
# querying it from the PR (when commit.url is set), or by building it from
# the commit title and hash.
#
def process_commit_push_branch(
def process_commit_infer_branch(
self,
*,
commit: Commit,
) -> tuple[Commit, BranchPushResult]:
) -> str:
if commit.url:
pr = self.gh_get_pr(url=commit.url)
commit.branch = pr.head_branch
pr = self.gh_get_pr(url=commit.url) # likely a cache hit
return pr.head_branch
else:
commit.branch = self.build_branch_name(
return self.build_branch_name(
title=commit.title,
commit_hash=commit.hash,
)
pushed = self.git_push_branch(branch=commit.branch, hash=commit.hash)
return commit, pushed

#
# Updates PR fields:
Expand All @@ -495,16 +530,12 @@ class Main:
pr_numbers: list[int] = []
pr_number_current: int | None = None
for c in commits:
assert (
c.url is not None
), f"commit {c.hash} PR URL is expected to be in the message at this point"
assert c.url is not None
if m := re.search(r"/(\d+)$", c.url):
pr_numbers.append(int(m.group(1)))
if c.hash == commit.hash:
pr_number_current = int(m.group(1))
assert (
commit.url is not None
), f"commit {commit.hash} PR URL is expected to be in the message at this point"
assert commit.url is not None
return self.gh_update_pr(
url=commit.url,
base_branch=prev_commit.branch if prev_commit else None,
Expand Down Expand Up @@ -864,9 +895,16 @@ class Main:
return commits

#
# Pushes a branch to remote GitHub.
# Pushes multiple branches atomically to remote GitHub. Returns a dict
# mapping branch names to their push results.
#
def git_push_branch(self, *, branch: str, hash: str) -> BranchPushResult:
def git_push_branches(
self,
*,
branches: list[Branch],
) -> dict[str, BranchPushResult]:
if not branches:
return {}
# Git push is a quick no-op on GitHub end if the branch isn't changed
# (it prints "Everything up-to-date"), so we always push and then verify
# the output for the status (instead of fetching from the remote and
Expand All @@ -877,15 +915,26 @@ class Main:
"push",
"-f",
self.remote,
f"{hash}:refs/heads/{branch}",
*[f"{branch.hash}:refs/heads/{branch.branch}" for branch in branches],
],
stderr_to_stdout=True,
)
return (
"up-to-date"
if re.match(r"^[^\n]+up-to-date", out, flags=re.S)
else "pushed"
)
# If the hash is NOT mentioned in the output, it's either a short
# "Everything up-to-date" message (which means that ALL branches are
# unchanged), or THIS particular branch is up-to-date. I.e. if a branch
# is changed, git always prints its hash in the output, on one of the
# following formats:
# 1. * [new branch] 10dc4f6 -> grok/...
# 2. + 10dc4f6...b28d03e 10dc4f6 -> grok/... (forced update)
results: dict[str, BranchPushResult] = {
branch.branch: "up-to-date" for branch in branches
}
for branch in branches:
for line in out.splitlines():
if branch.hash in line:
results[branch.branch] = "pushed"
break
return results

#
# Runs an interactive rebase with the provided shell command.
Expand Down Expand Up @@ -1147,7 +1196,7 @@ class Main:
# Prints a status after a commit message was updated locally.
#
def print_commit_message_updated(self):
print(f" ── updated commit message")
print(f" ── added PR URL to commit's message to bind them")
sys.stdout.flush()

#
Expand Down Expand Up @@ -1282,7 +1331,15 @@ class Main:
def debug_log_text(self, *, text: str):
try:
with open(DEBUG_FILE, "a+") as file:
file.write(f"=== {datetime.now()}\n")
file.write(
f"=== {datetime.now()}"
+ (
f" (in rebase interactive, {self.in_rebase_interactive.commit_index_one_based}/{self.in_rebase_interactive.total_commits_in_stack})"
if self.in_rebase_interactive
else ""
)
+ "\n"
)
file.write(f"{text.rstrip()}\n")
file.write("\n")
except:
Expand Down
Loading