Summary

Add empty file handling in JujutsuPatcher.

Review Request #14760 — Created Jan. 6, 2026 and submitted Jan. 9, 2026, 11:20 a.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

In my previous change that updated the patching infrastructure to add
support for patching binary files, I changed the top-level Patcher class
so it would (when supported) call apply_patch_for_empty_files. This
caused a regression with Jujutsu, where we had no implementation of
that. I didn't catch this because it was hidden behind a conditional
which was false in the case of unit tests, since we weren't
instantiating the client with a capabilities object.

This change adds an implementation of that method for Jujutsu which
works similar to the SVN and Perforce implementations. Mercurial and Git
are otherwise immune because they use SCM-native patching which handles
empty files itself (hg import and git apply, respectively).

Some of the jj unit tests were incorrectly named, saying they included
empty files when they didn't. I've fixed those up, deleted a test which
did not do anything unique, and added two new tests similar to ones we
have in GitPatcherTests for ensuring that empty files work correctly.

To be complete, I've also added the default capabilities object to the
Git, Mercurial, and Perforce tests, just to ensure that the result of
supports_empty_files() will be correct when running the tests.

Testing Done

Ran unit tests.

Commits

Summary	ID
Add empty file handling in JujutsuPatcher. In my previous change that updated the patching infrastructure to add support for patching binary files, I changed the top-level Patcher class so it would (when supported) call `apply_patch_for_empty_files`. This caused a regression with Jujutsu, where we had no implementation of that. I didn't catch this because it was hidden behind a conditional which was false in the case of unit tests, since we weren't instantiating the client with a capabilities object. This change adds an implementation of that method for Jujutsu which works similar to the SVN and Perforce implementations. Mercurial and Git are otherwise immune because they use SCM-native patching which handles empty files itself (`hg import` and `git apply`, respectively). Some of the jj unit tests were incorrectly named, saying they included empty files when they didn't. I've fixed those up, deleted a test which did not do anything unique, and added two new tests similar to ones we have in `GitPatcherTests` for ensuring that empty files work correctly. To be complete, I've also added the default capabilities object to the Git, Mercurial, and Perforce tests, just to ensure that the result of `supports_empty_files()` will be correct when running the tests. Testing Done: Ran unit tests.	zyloxzouusozptwmwyrvtswmorwxovus

Summary

Add empty file handling in JujutsuPatcher.

In my previous change that updated the patching infrastructure to add support for patching binary files, I changed the top-level Patcher class so it would (when supported) call `apply_patch_for_empty_files`. This caused a regression with Jujutsu, where we had no implementation of that. I didn't catch this because it was hidden behind a conditional which was false in the case of unit tests, since we weren't instantiating the client with a capabilities object. This change adds an implementation of that method for Jujutsu which works similar to the SVN and Perforce implementations. Mercurial and Git are otherwise immune because they use SCM-native patching which handles empty files itself (`hg import` and `git apply`, respectively). Some of the jj unit tests were incorrectly named, saying they included empty files when they didn't. I've fixed those up, deleted a test which did not do anything unique, and added two new tests similar to ones we have in `GitPatcherTests` for ensuring that empty files work correctly. To be complete, I've also added the default capabilities object to the Git, Mercurial, and Perforce tests, just to ensure that the result of `supports_empty_files()` will be correct when running the tests. Testing Done: Ran unit tests.

zyloxzouusozptwmwyrvtswmorwxovus

Issues

Description	From	Last Updated
continuation line over-indented for visual indent Column: 64 Error code: E127	reviewbot	Jan. 6, 2026, 2:44 p.m.
blank line at end of file Column: 1 Error code: W391	reviewbot	Jan. 6, 2026, 2:44 p.m.
This should say "deleted empty files"	chipx86	Jan. 6, 2026, 4:18 p.m.
What raises this?	chipx86	Jan. 6, 2026, 4:18 p.m.
Blank line between these.	chipx86	Jan. 6, 2026, 4:18 p.m.
We're decoding multiple times, which is a bit wasteful. We should decode in a separate step, or an internal comprehension: …	chipx86	Jan. 6, 2026, 4:19 p.m.
Blank line here.	chipx86	Jan. 6, 2026, 4:20 p.m.
Blank line here.	chipx86	Jan. 6, 2026, 4:20 p.m.
This log should probably include the filename in quotes followed by the error message.	chipx86	Jan. 6, 2026, 4:20 p.m.
No trailing period here.	chipx86	Jan. 6, 2026, 4:20 p.m.
Blank line between statements and blocks.	chipx86	Jan. 6, 2026, 4:21 p.m.
We don't normally do an explicit mode=, just leaving it as positional. Here and below.	chipx86	Jan. 6, 2026, 4:23 p.m.
The second keyword argument should be on its own line.	chipx86	Jan. 6, 2026, 4:22 p.m.
No trailing period here.	chipx86	Jan. 6, 2026, 4:22 p.m.
Blank line between statements and blocks.	chipx86	Jan. 6, 2026, 4:22 p.m.
Same comment re: one keyword argument per line.	chipx86	Jan. 6, 2026, 4:22 p.m.
Blank line here.	chipx86	Jan. 6, 2026, 4:23 p.m.
Is there a chance that one file deletion fails, but the others succeed? In that case we'd return True from …	maubin	Jan. 8, 2026, 8:48 a.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/jujutsu.py (Diff revision 1)

The issue has been dropped. Show all issues

continuation line over-indented for visual indent

Column: 64
Error code: E127

rbtools/clients/tests/test_jujutsu.py (Diff revision 1)
The issue has been dropped. Show all issues
```
blank line at end of file

Column: 1
Error code: W391
```

Commits:

	Summary	ID
	Add empty file handling in JujutsuPatcher. In my previous change that updated the patching infrastructure to add support for patching binary files, I changed the top-level Patcher class so it would (when supported) call `apply_patch_for_empty_files`. This caused a regression with Jujutsu, where we had no implementation of that. I didn't catch this because it was hidden behind a conditional which was false in the case of unit tests, since we weren't instantiating the client with a capabilities object. This change adds an implementation of that method for Jujutsu which works similar to the SVN and Perforce implementations. Mercurial and Git are otherwise immune because they use SCM-native patching which handles empty files itself (`hg import` and `git apply`, respectively). Some of the jj unit tests were incorrectly named, saying they included empty files when they didn't. I've fixed those up, deleted a test which did not do anything unique, and added two new tests similar to ones we have in `GitPatcherTests` for ensuring that empty files work correctly. To be complete, I've also added the default capabilities object to the Git, Mercurial, and Perforce tests, just to ensure that the result of `supports_empty_files()` will be correct when running the tests. Testing Done: Ran unit tests.	zyloxzouusozptwmwyrvtswmorwxovus
	Add empty file handling in JujutsuPatcher. In my previous change that updated the patching infrastructure to add support for patching binary files, I changed the top-level Patcher class so it would (when supported) call `apply_patch_for_empty_files`. This caused a regression with Jujutsu, where we had no implementation of that. I didn't catch this because it was hidden behind a conditional which was false in the case of unit tests, since we weren't instantiating the client with a capabilities object. This change adds an implementation of that method for Jujutsu which works similar to the SVN and Perforce implementations. Mercurial and Git are otherwise immune because they use SCM-native patching which handles empty files itself (`hg import` and `git apply`, respectively). Some of the jj unit tests were incorrectly named, saying they included empty files when they didn't. I've fixed those up, deleted a test which did not do anything unique, and added two new tests similar to ones we have in `GitPatcherTests` for ensuring that empty files work correctly. To be complete, I've also added the default capabilities object to the Git, Mercurial, and Perforce tests, just to ensure that the result of `supports_empty_files()` will be correct when running the tests. Testing Done: Ran unit tests.	zyloxzouusozptwmwyrvtswmorwxovus

Diff:

Revision 2 (+444 -140)

Show changes

	rbtools/clients/jujutsu.py
	rbtools/clients/tests/test_git.py
	rbtools/clients/tests/test_jujutsu.py
	rbtools/clients/tests/test_mercurial.py
	rbtools/clients/tests/test_p4.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should say "deleted empty files"
```
rbtools/clients/jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
What raises this?
```
1. david Jan. 6, 2026, 4:24 p.m.
  Copy/paste from the parent class docstring.
rbtools/clients/jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

rbtools/clients/jujutsu.py (Diff revision 2)

The issue has been resolved. Show all issues

We're decoding multiple times, which is a bit wasteful. We should decode in a separate step, or an internal comprehension:

added_files = [
    filename
    for filename in (
        filename.decode('utf-8')
        for filename in added_files_re.findall(patch_content)
    )
    if filename not in binary_file_paths
]

rbtools/clients/jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line here.
```
rbtools/clients/jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line here.
```

rbtools/clients/jujutsu.py (Diff revision 2)

The issue has been resolved. Show all issues

This log should probably include the filename in quotes followed by the error message.

rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No trailing period here.
```
rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between statements and blocks.
```

rbtools/clients/tests/test_jujutsu.py (Diff revision 2)

The issue has been dropped. Show all issues

We don't normally do an explicit mode=, just leaving it as positional. Here and below.

david Jan. 6, 2026, 4:24 p.m.

When adding the encoding argument, I've been turning mode into a kwarg for clarity. I think it reads much better than just a stringly-typed positional arg.

rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
The second keyword argument should be on its own line.
```
rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No trailing period here.
```
rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between statements and blocks.
```
rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same comment re: one keyword argument per line.
```
rbtools/clients/tests/test_jujutsu.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line here.
```

Commits:

	Summary	ID
	Add empty file handling in JujutsuPatcher. In my previous change that updated the patching infrastructure to add support for patching binary files, I changed the top-level Patcher class so it would (when supported) call `apply_patch_for_empty_files`. This caused a regression with Jujutsu, where we had no implementation of that. I didn't catch this because it was hidden behind a conditional which was false in the case of unit tests, since we weren't instantiating the client with a capabilities object. This change adds an implementation of that method for Jujutsu which works similar to the SVN and Perforce implementations. Mercurial and Git are otherwise immune because they use SCM-native patching which handles empty files itself (`hg import` and `git apply`, respectively). Some of the jj unit tests were incorrectly named, saying they included empty files when they didn't. I've fixed those up, deleted a test which did not do anything unique, and added two new tests similar to ones we have in `GitPatcherTests` for ensuring that empty files work correctly. To be complete, I've also added the default capabilities object to the Git, Mercurial, and Perforce tests, just to ensure that the result of `supports_empty_files()` will be correct when running the tests. Testing Done: Ran unit tests.	zyloxzouusozptwmwyrvtswmorwxovus
	Add empty file handling in JujutsuPatcher. In my previous change that updated the patching infrastructure to add support for patching binary files, I changed the top-level Patcher class so it would (when supported) call `apply_patch_for_empty_files`. This caused a regression with Jujutsu, where we had no implementation of that. I didn't catch this because it was hidden behind a conditional which was false in the case of unit tests, since we weren't instantiating the client with a capabilities object. This change adds an implementation of that method for Jujutsu which works similar to the SVN and Perforce implementations. Mercurial and Git are otherwise immune because they use SCM-native patching which handles empty files itself (`hg import` and `git apply`, respectively). Some of the jj unit tests were incorrectly named, saying they included empty files when they didn't. I've fixed those up, deleted a test which did not do anything unique, and added two new tests similar to ones we have in `GitPatcherTests` for ensuring that empty files work correctly. To be complete, I've also added the default capabilities object to the Git, Mercurial, and Perforce tests, just to ensure that the result of `supports_empty_files()` will be correct when running the tests. Testing Done: Ran unit tests.	zyloxzouusozptwmwyrvtswmorwxovus

Diff:

Revision 3 (+464 -140)

Show changes

	rbtools/clients/jujutsu.py
	rbtools/clients/tests/test_git.py
	rbtools/clients/tests/test_jujutsu.py
	rbtools/clients/tests/test_mercurial.py
	rbtools/clients/tests/test_p4.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/jujutsu.py (Diff revision 3)

The issue has been dropped. Show all issues

Is there a chance that one file deletion fails, but the others succeed? In that case we'd return True from this. Is that what we want? Or do we want to return False if at least one file fails.

david Jan. 8, 2026, 8:48 a.m.

It's possible. Other SCM backends that implement this return True if at least one thing succeeds, so I matched that behavior. In the case that one fails the user will see that error, but the patch as a whole will be semi-applied.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (eb1cac0)