Discharge7443/brave-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[cr150][brockit] Fix fixup! block message parsing (#36610)

Browse Source 
[cr150][brockit] Fix handling of `fixup!` block messages

This PR fixes how we are handling the block messages for `fixup!`, as
the parsing for them was always assuming that they would receive the
label `will be skipped:` for not having any message in them, but
actually that doesn't appear to be the case with every `fixup!`. With
this change, `fixup!` blocks that have no message are now discarded too.

Bug: https://github.com/brave/brave-browser/issues/55466
This commit is contained in:
cdesouza-chromium
2026-05-21 17:48:00 +01:00
committed by GitHub
parent 4a42f1e6c5
commit 3213961ae5
2 changed files with 94 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tools/cr/rebase_v2.py
+6
View File
@@ -758,6 +758,12 @@ class MsgBlock:
blocks[-1]._extend_message(msg)
elif msg:
blocks.append(MsgBlock(full_message=msg, note=note))
elif note is not None and note.command == "fixup!":
# A bare `# fixup!` block carries no message of its own
# -- git is just echoing the marker for context (this
# also happens with stacked `# fixup! fixup! …` markers).
# Treat it the same as a `will be skipped:` block.
continue
else:
raise EditorRecoverableFailure(
f'Unexpected empty message in block. Block lines: '
tools/cr/rebase_v2_test.py
+88
View File
@@ -818,6 +818,44 @@ class MessageWriterTest(unittest.TestCase):
'Update from Chromium 1.0.0.1 to Chromium 1.0.0.2',
])
def test_bare_fixup_block_is_skipped(self):
"""When the user has stacked `fixup!` commits on top of an
already-fixup commit, git emits the marker-only `# fixup!`
(or `# fixup! fixup! ...`) inside a regular `# This is the
commit message #N:` header with no body. Those blocks carry
nothing to merge -- treat them like `will be skipped:` rather
than failing the parse."""
path = self._file(
'# This is a combination of 4 commits.\n'
'# This is the 1st commit message:\n'
'\n'
'Conflict-resolved patches from Chromium 1.0.0.0 to '
'Chromium 1.0.0.1.\n'
'\n'
'# This is the commit message #2:\n'
'\n'
'# fixup! Conflict-resolved patches from Chromium 1.0.0.0 to '
'Chromium 1.0.0.1.\n'
'\n'
'# This is the commit message #3:\n'
'\n'
'# fixup! fixup! Conflict-resolved patches from Chromium 1.0.0.0 '
'to Chromium 1.0.0.1.\n'
'\n'
'# This is the commit message #4:\n'
'\n'
'Conflict-resolved patches from Chromium 1.0.0.2 to '
'Chromium 1.0.0.3.\n')
writer = rebase_v2.MessageWriter.parse(path)
self.assertEqual(self._messages(writer), [
'Conflict-resolved patches from Chromium 1.0.0.0 to '
'Chromium 1.0.0.1.',
'Conflict-resolved patches from Chromium 1.0.0.2 to '
'Chromium 1.0.0.3.',
])
def test_git_footer_terminates_parsing(self):
"""Anything after `# Please enter the commit message ...` is
ignored, including stale `# This is ...` lines from git's status
@@ -1018,6 +1056,56 @@ class RebaseV2ExecuteTest(unittest.TestCase):
'[cr148] Some unrelated feature commit',
])
def _commit_fixup(self, target: str = 'HEAD') -> str:
"""Stages a unique gen-N.txt and commits it with `git commit
--fixup=<target>`. Mirrors what `git commit --fixup` produces in
the wild: a commit whose subject is `fixup! <target subject>`
and whose body is empty. Stacking a second call (with the
previous fixup as target) yields a `fixup! fixup! <subject>`
subject -- the marker-only blocks that broke `MsgBlock.parse`."""
self._commit_counter += 1
self.repo.write_and_stage_file(f'gen-{self._commit_counter}.txt',
f'fixup {self._commit_counter}\n',
self.repo.brave)
self.repo._run_git_command(['commit', f'--fixup={target}'],
self.repo.brave)
return self.repo._run_git_command(['rev-parse', 'HEAD'],
self.repo.brave)
def test_v2_squash_minor_bumps_with_stacked_fixup_commits(self):
"""Regression: `git commit --fixup=<conflict-resolved commit>` (and
a stacked fixup of that fixup) produces commits whose message is
only the autosquash marker -- `fixup! …` and `fixup! fixup! …`
with empty bodies. After `--autosquash` chains them next to the
pinned target and v2's `squash_minor_bumps` collapses the whole
group, git emits those as marker-only `# fixup!` blocks inside
the squash editor file. `MsgBlock.parse` used to raise
`EditorRecoverableFailure` for them; the rebase must instead
complete and keep the latest pinned subject."""
scenario = self._seed_bump_branch()
first_pinned = self._commit_with_file(
'Conflict-resolved patches from Chromium 1.0.0.1 to '
'Chromium 1.0.0.2.')
inner_fixup = self._commit_fixup(target=first_pinned)
self._commit_fixup(target=inner_fixup)
self._commit_with_file(
'Conflict-resolved patches from Chromium 1.0.0.2 to '
'Chromium 1.0.0.3.')
brockit.Rebase().execute(from_ref=scenario['v101'],
to_ref=scenario['v102'],
recommit=False,
discard_regen_changes=False,
squash_minor_bumps=True,
v2=True)
subjects = self._git_log_subjects(scenario['v102'] + '..HEAD')
self.assertEqual(subjects, [
'Conflict-resolved patches from Chromium 1.0.0.2 to '
'Chromium 1.0.0.3.',
'Add brave-only feature.txt',
])
def test_v2_recommit_amends_first_commit(self):
"""`--recommit` is reused from v1 -- v2 doesn't change its
semantics, just confirms it still works when `v2=True`."""