Summary

Add infrastructure for binary file patching in rbt patch.

Review Request #14711 — Created Nov. 28, 2025 and updated Jan. 7, 2026, 8:50 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This adds the core infrastructure needed to support binary file patching
when applying review request diffs using rbt patch.

rbtools.diffs.patches.BinaryFilePatch represents binary files in the
diff, and can lazily load their content as-needed.

rbtools.diffs.patcher.Patcher now has several methods to apply binary
files in various ways. They can be overridden by SCM-specific patchers
to perform file operations as necessary.

rbtools.diffs.patches.PatchResult will now track which binary files
were applied (or failed).

The rbt patch command has been updated to:

Fetch binary file metadata from diff and commit resources.
Pass binary files to Patch instances.
Report binary file application results
Add the new --no-binary option, which skips patching binary files.

Subsequent changes will add support for individual SCM systems.

Testing Done


Ran unit tests.
Used this in conjunction with later changes to actually apply patches

  that include binary files.

Commits

Summary	ID
Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx

Summary

Add infrastructure for binary file patching in rbt patch.

This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.

mxnmkxtkuwpysslkkonvosuyumspvzsx

Issues

Description	From	Last Updated
Since this is only used here internally, it might be worth just letting this live in TYPE_CHECKING.	chipx86	Dec. 30, 2025, 10:26 a.m.
Can you add double-backticks around this and a period?	chipx86	Dec. 30, 2025, 10:26 a.m.
Missing a period.	maubin	Dec. 30, 2025, 10:39 a.m.
This might be a bit easier to follow if we remove a level of nesting, like: if p_num == 0: …	chipx86	Dec. 30, 2025, 10:26 a.m.
This is a small regex. Might just be worth defining it once globally instead of per-class. Otherwise, if we're likely …	chipx86	Dec. 30, 2025, 10:28 a.m.
Can we make reverted keyword-only? Same with the other similar methods.	chipx86	Dec. 30, 2025, 10:29 a.m.
Should this be Sequence?	maubin	Dec. 30, 2025, 10:56 a.m.
Missing parens around the union.	chipx86	Dec. 30, 2025, 10:29 a.m.
Missing a trailing period.	chipx86	Dec. 30, 2025, 10:30 a.m.
Missing a blank line before the if.	chipx86	Dec. 30, 2025, 10:30 a.m.
This is going to overwrite binary_files_applied each time, preserving only the last filename. append() might be the right thing, but …	chipx86	Dec. 30, 2025, 10:32 a.m.
Similar issue with json.add. It's going to overwrite each time.	chipx86	Dec. 30, 2025, 10:32 a.m.
We could do a walrus operator here so we don't have to access patch.binary_files twice.	maubin	Dec. 30, 2025, 10:55 a.m.
We're inconsistent with localization in RBTools. We probably should work toward localizing all new strings. I just picked one example, …	chipx86	Dec. 30, 2025, 11:12 a.m.
This sort of reads like "add a file (to the patcher)", but it's really "handle a file add". I wonder …	chipx86	Dec. 30, 2025, 11:27 a.m.
This needs a Version Added. Same with the functions below.	chipx86	Dec. 30, 2025, 10:33 a.m.
Can we call this fp, so it's consistent with most other open calls? Other file information objects in this change …	chipx86	Dec. 30, 2025, 10:33 a.m.
To keep this in alphabetical order, these should be swapped.	chipx86	Dec. 30, 2025, 10:33 a.m.
Should we make a TypeAlias for this?	maubin	Dec. 30, 2025, 10:48 a.m.
Missing 'moved'.	maubin	Dec. 30, 2025, 10:46 a.m.
These comments are missing trailing periods.	chipx86	Dec. 30, 2025, 10:34 a.m.
Missing period. Same with others in the file.	chipx86	Dec. 30, 2025, 10:34 a.m.
Missing period and blank line.	chipx86	Dec. 30, 2025, 10:34 a.m.
Can we stick to one per line?	chipx86	Dec. 30, 2025, 10:35 a.m.
Can we make these keyword-only?	chipx86	Dec. 30, 2025, 10:35 a.m.
This needs a Version Added.	chipx86	Dec. 30, 2025, 10:36 a.m.
It's not really clear what this is ultimately doing. Maybe worth just making a function for this?	chipx86	Dec. 30, 2025, 10:45 a.m.
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	Dec. 30, 2025, 11:39 a.m.
line too long (87 > 79 characters) Column: 80 Error code: E501	reviewbot	Dec. 30, 2025, 11:39 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Add a utility function for testing binary file patch downloads.

Commits:

	Summary	ID
	Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). * The `patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx
	Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx

Diff:

Revision 2 (+2354 -104)

Show changes

	rbtools/clients/base/scmclient.py
	rbtools/commands/patch.py
	rbtools/diffs/patcher.py
	rbtools/diffs/patches.py
	rbtools/diffs/tests/test_patch.py
	rbtools/diffs/tests/test_patcher.py
	rbtools/testing/testcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/base/scmclient.py (Diff revision 2)

The issue has been resolved. Show all issues

Since this is only used here internally, it might be worth just letting this live in TYPE_CHECKING.

rbtools/clients/base/scmclient.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add double-backticks around this and a period?
```

rbtools/clients/base/scmclient.py (Diff revision 2)

The issue has been resolved. Show all issues

This might be a bit easier to follow if we remove a level of nesting, like:

if p_num == 0:
    return file_or_files

# Rest here

rbtools/clients/base/scmclient.py (Diff revision 2)

The issue has been dropped. Show all issues

This is a small regex. Might just be worth defining it once globally instead of per-class.

Otherwise, if we're likely to access this more than we set it, this can just be a try/except, catching AttributeError.

david Dec. 30, 2025, 11:28 a.m.

By the time we're stripping prefixes from diffs we only have one type of SCMClient class anyway. Moving it globally doesn't buy us anything.

rbtools/commands/patch.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we make reverted keyword-only?

Same with the other similar methods.

rbtools/commands/patch.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing parens around the union.
```
rbtools/commands/patch.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```
rbtools/commands/patch.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a blank line before the if.
```

rbtools/commands/patch.py (Diff revision 2)

The issue has been resolved. Show all issues

This is going to overwrite binary_files_applied each time, preserving only the last filename.

append() might be the right thing, but I think you probably just want:

self.json.add('binary_files_applied', patch_result.binary_applied)

rbtools/commands/patch.py (Diff revision 2)

The issue has been resolved. Show all issues

Similar issue with json.add. It's going to overwrite each time.

rbtools/diffs/patcher.py (Diff revision 2)

The issue has been resolved. Show all issues

We're inconsistent with localization in RBTools. We probably should work toward localizing all new strings. I just picked one example, but this applies more generally.

rbtools/diffs/patcher.py (Diff revision 2)

The issue has been resolved. Show all issues

This sort of reads like "add a file (to the patcher)", but it's really "handle a file add". I wonder if we should call this handle_add_file()? Same thought for move/remove/write.

rbtools/diffs/patcher.py (Diff revision 2)

The issue has been resolved. Show all issues

This needs a Version Added.

Same with the functions below.

rbtools/diffs/patcher.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we call this fp, so it's consistent with most other open calls? Other file information objects in this change use f.

rbtools/diffs/patches.py (Diff revision 2)
The issue has been resolved. Show all issues
```
To keep this in alphabetical order, these should be swapped.
```
rbtools/diffs/tests/test_patch.py (Diff revision 2)
The issue has been resolved. Show all issues
```
These comments are missing trailing periods.
```
rbtools/diffs/tests/test_patcher.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing period.
Same with others in the file.
```
rbtools/diffs/tests/test_patcher.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing period and blank line.
```
rbtools/diffs/tests/test_patcher.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we stick to one per line?
```

rbtools/testing/testcase.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we make these keyword-only?

rbtools/testing/testcase.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs a Version Added.
```

rbtools/testing/testcase.py (Diff revision 2)

The issue has been resolved. Show all issues

It's not really clear what this is ultimately doing. Maybe worth just making a function for this?

rbtools/clients/base/scmclient.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a period.
```
rbtools/commands/patch.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should this be Sequence?
```

rbtools/diffs/patcher.py (Diff revision 2)

The issue has been resolved. Show all issues

We could do a walrus operator here so we don't have to access patch.binary_files twice.

rbtools/diffs/patches.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should we make a TypeAlias for this?
```
rbtools/diffs/patches.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing 'moved'.
```

Commits:

	Summary	ID
	Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx
	Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx

Diff:

Revision 3 (+2482 -112)

Show changes

	dev-requirements.txt
	rbtools/clients/base/scmclient.py
	rbtools/commands/patch.py
	rbtools/diffs/patcher.py
	rbtools/diffs/patches.py
	rbtools/diffs/tests/test_patch.py
	rbtools/diffs/tests/test_patcher.py
	rbtools/testing/testcase.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/patch.py (Diff revision 3)
The issue has been dropped. Show all issues
```
line too long (81 > 79 characters)

Column: 80
Error code: E501
```
rbtools/commands/patch.py (Diff revision 3)
The issue has been dropped. Show all issues
```
line too long (87 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx
	Add infrastructure for binary file patching in rbt patch. This adds the core infrastructure needed to support binary file patching when applying review request diffs using `rbt patch`. `rbtools.diffs.patches.BinaryFilePatch` represents binary files in the diff, and can lazily load their content as-needed. `rbtools.diffs.patcher.Patcher` now has several methods to apply binary files in various ways. They can be overridden by SCM-specific patchers to perform file operations as necessary. `rbtools.diffs.patches.PatchResult` will now track which binary files were applied (or failed). The `rbt patch` command has been updated to: - Fetch binary file metadata from diff and commit resources. - Pass binary files to `Patch` instances. - Report binary file application results - Add the new `--no-binary` option, which skips patching binary files. Subsequent changes will add support for individual SCM systems. Testing Done: - Ran unit tests. - Used this in conjunction with later changes to actually apply patches that include binary files.	mxnmkxtkuwpysslkkonvosuyumspvzsx

Diff:

Revision 4 (+2482 -112)

Show changes

	dev-requirements.txt
	rbtools/clients/base/scmclient.py
	rbtools/commands/patch.py
	rbtools/diffs/patcher.py
	rbtools/diffs/patches.py
	rbtools/diffs/tests/test_patch.py
	rbtools/diffs/tests/test_patcher.py
	rbtools/testing/testcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```