From d6fce997a2bd0c3a24e07c9973b7230b0026fe64 Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Mon, 22 Jun 2026 18:30:09 -0700 Subject: [PATCH] amend: Fix last-touched functionality last-touched will amend the file into the correct commit, but incorrectly then removes the changes immediately when replaying the rest of the stack on top. This was not caught by tests because those only checked the exact commit and file, without checking that the rest of the stack is correct. Fix this by reimplementing with the merge-tree engine, which is more modern and faster anyway. Update tests to catch this case. --- revup/amend.py | 54 ++++++++++++++++++++++++--------------------- revup/git.py | 39 ++++++++++++++++++++++++++++++++ tests/test_amend.py | 46 +++++++++++++++++++++++++++++++------- 3 files changed, 106 insertions(+), 33 deletions(-) diff --git a/revup/amend.py b/revup/amend.py index c839d39..04489ae 100644 --- a/revup/amend.py +++ b/revup/amend.py @@ -190,33 +190,37 @@ async def rebuild_stack_last_touched( _, path = line.split("\t", 1) staged_entries[path] = line - tmp_index = git_ctx.get_scratch_dir() + "/last_touched_index" - new_commit = GitCommitHash(stack[0].parents[0]) if stack[0].parents else GitCommitHash("") for i, commit_obj in enumerate(stack): - if i in commit_to_files: - idx_env = {"GIT_INDEX_FILE": tmp_index} - await git_ctx.git("read-tree", commit_obj.tree, env=idx_env) - update_lines = [staged_entries[f] for f in commit_to_files[i]] - await git_ctx.git( - "update-index", - "--index-info", - env=idx_env, - input_str="\n".join(update_lines) + "\n", - ) - new_tree = GitTreeHash(await git_ctx.git_stdout("write-tree", env=idx_env)) - amended = CommitHeader(new_tree, [new_commit]) - amended.author_name = commit_obj.author_name - amended.author_email = commit_obj.author_email - amended.author_date = commit_obj.author_date - amended.committer_name = commit_obj.committer_name - amended.committer_email = commit_obj.committer_email - # Refresh committer date, like git commit --amend. - amended.committer_date = "" - amended.commit_msg = commit_obj.commit_msg - new_commit = await git_ctx.commit_tree(amended) - else: - new_commit = await replay_cherry_pick(git_ctx, commit_obj, new_commit) + # Cherry-pick onto the rebuilt parent first, so amendments to earlier + # commits propagate forward through the rest of the stack. + rebuilt_parent = new_commit + new_commit = await replay_cherry_pick(git_ctx, commit_obj, new_commit) + if i not in commit_to_files: + continue + + # Build a tree of just this commit's staged files and overlay it onto the + # cherry-picked commit. The merge base is the empty tree, so --theirs will + # always take the incoming version and will never conflict. + overlay = await git_ctx.make_tree_from_index_entries( + [staged_entries[f] for f in commit_to_files[i]] + ) + amended = CommitHeader(GitTreeHash(""), [rebuilt_parent]) + amended.author_name = commit_obj.author_name + amended.author_email = commit_obj.author_email + amended.author_date = commit_obj.author_date + amended.committer_name = commit_obj.committer_name + amended.committer_email = commit_obj.committer_email + # Refresh committer date, like git commit --amend. + amended.committer_date = "" + amended.commit_msg = commit_obj.commit_msg + new_commit = await git_ctx.merge_tree_commit( + new_commit, + GitCommitHash(overlay), + amended, + GitCommitHash(await git_ctx.empty_tree()), + "theirs", + ) return new_commit diff --git a/revup/git.py b/revup/git.py index bb20739..adda45c 100644 --- a/revup/git.py +++ b/revup/git.py @@ -582,6 +582,45 @@ async def commit_tree(self, commit_info: CommitHeader) -> GitCommitHash: ret = await self.git_stdout(*commit_tree_args, env=git_env) return GitCommitHash(ret) + @lru_cache(maxsize=None) + async def empty_tree(self) -> GitTreeHash: + """ + Return the hash of the empty tree, computed so it works under any hash algorithm. + """ + return GitTreeHash(await self.git_stdout("mktree", input_str="")) + + async def make_tree_from_index_entries(self, entries: List[str]) -> GitTreeHash: + """ + Build a tree from raw `ls-files --stage` formatted index entries + (` \\t`), without touching any index. + + `git mktree` only builds a single tree level and rejects paths with + slashes, so nested paths are grouped by directory and the trees are + built bottom-up, writing one subtree object per directory. + """ + # Nested dict: dir name -> subtree dict, plus "" -> list of blob lines + # for the files directly in that directory. + root: Dict[str, Any] = {} + for entry in entries: + meta, path = entry.split("\t", 1) + mode, blob, _stage = meta.split(" ") + *dirs, name = path.split("/") + node = root + for d in dirs: + node = node.setdefault(d, {}) + node.setdefault("", []).append(f"{mode} blob {blob}\t{name}") + + async def build(node: Dict[str, Any]) -> GitTreeHash: + lines = list(node.get("", [])) + for name, child in node.items(): + if name == "": + continue + subtree = await build(child) + lines.append(f"040000 tree {subtree}\t{name}") + return GitTreeHash(await self.git_stdout("mktree", input_str="\n".join(lines) + "\n")) + + return await build(root) + @lru_cache(maxsize=None) async def merge_tree( self, diff --git a/tests/test_amend.py b/tests/test_amend.py index 5bb729e..49132dd 100644 --- a/tests/test_amend.py +++ b/tests/test_amend.py @@ -777,19 +777,23 @@ async def test_amends_file_into_last_commit_that_touched_it(self): async with GitTestEnvironment() as env: await env.commit("root", {"root.txt": "r"}) await env.git_ctx.git("branch", "origin/main", "HEAD") + # a.txt is touched by both "first" and "second"; the amend must land + # in "second" (the last to touch it), not "first". await env.commit("first\n\nTopic: alpha", {"a.txt": "v1"}) - await env.commit("second\n\nTopic: beta", {"b.txt": "v1"}) + await env.commit("second\n\nTopic: beta", {"a.txt": "v2", "b.txt": "v1"}) + await env.commit("third\n\nTopic: gamma", {"c.txt": "v1"}) - await env.stage_file("a.txt", "v2") + await env.stage_file("a.txt", "v3") args = make_amend_args(last_touched=True, parse_topics=True) await amend.main(args, env.git_ctx) - # a.txt should be amended into "first" (HEAD~1), not HEAD - content = await env.get_file_at_commit("a.txt", "HEAD~1") - assert content == "v2" + # a.txt should be amended into "second" (HEAD~1), not "first" (HEAD~2) + assert await env.get_file_at_commit("a.txt", "HEAD~2") == "v1" + assert await env.get_file_at_commit("a.txt", "HEAD~1") == "v3" + # ...and propagate forward to "third" (HEAD) + assert await env.get_file_at_commit("a.txt", "HEAD") == "v3" # b.txt should be unchanged - content_b = await env.get_file_at_commit("b.txt", "HEAD") - assert content_b == "v1" + assert await env.get_file_at_commit("b.txt", "HEAD") == "v1" @async_test async def test_multiple_files_to_different_commits(self): @@ -810,6 +814,26 @@ async def test_multiple_files_to_different_commits(self): assert await env.get_file_at_commit("b.txt", "HEAD~1") == "b2" assert await env.get_file_at_commit("c.txt", "HEAD") == "c2" + @async_test + async def test_amended_change_propagates_forward(self): + async with GitTestEnvironment() as env: + await env.commit("root", {"root.txt": "r"}) + await env.git_ctx.git("branch", "origin/main", "HEAD") + # Nested path exercises the recursive mktree logic in + # make_tree_from_index_entries. + await env.commit("first\n\nTopic: alpha", {"sub/dir/a.txt": "v1"}) + await env.commit("second\n\nTopic: beta", {"b.txt": "b1"}) + + # sub/dir/a.txt is amended into "first"; the new content must carry + # through to the rebuilt "second" as well, with no leftover staged change. + await env.stage_file("sub/dir/a.txt", "v2") + args = make_amend_args(last_touched=True, parse_topics=True) + await amend.main(args, env.git_ctx) + + assert await env.get_file_at_commit("sub/dir/a.txt", "HEAD~1") == "v2" + assert await env.get_file_at_commit("sub/dir/a.txt", "HEAD") == "v2" + assert not await env.has_staged_changes() + @async_test async def test_file_not_in_stack_remains_staged(self): async with GitTestEnvironment() as env: @@ -916,14 +940,20 @@ async def test_works_with_subdirectories(self): async with GitTestEnvironment() as env: await env.commit("root", {"root.txt": "r"}) await env.git_ctx.git("branch", "origin/main", "HEAD") - await env.commit("first\n\nTopic: alpha", {"src/lib/a.txt": "a1"}) + # Two sibling files in the same subdirectory land in the same commit, + # exercising a mktree subtree built from multiple entries. + await env.commit( + "first\n\nTopic: alpha", {"src/lib/a.txt": "a1", "src/lib/c.txt": "c1"} + ) await env.commit("second\n\nTopic: beta", {"src/lib/b.txt": "b1"}) await env.stage_file("src/lib/a.txt", "a2") + await env.stage_file("src/lib/c.txt", "c2") args = make_amend_args(last_touched=True, parse_topics=True) await amend.main(args, env.git_ctx) assert await env.get_file_at_commit("src/lib/a.txt", "HEAD~1") == "a2" + assert await env.get_file_at_commit("src/lib/c.txt", "HEAD~1") == "c2" assert await env.get_file_at_commit("src/lib/b.txt", "HEAD") == "b1" @async_test