Skip to content
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.

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

Fix empty files can't be selected in builtin diff editor #3481

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

emesterhazy
Copy link
Contributor

This fixes several issues that made working with empty files difficult using the builtin diff editor.

  1. The scm-record library used for the editor expects each file to have at least one section. For empty files this should be a file mode section. jj wasn't rendering this mode section, which prevented empty files from being selected at all.

  2. The scm-record library returns SelectedContents::Absent if the file has no contents after the diff editor is invoked. This can be because of several reasons: 1) the file is new and empty; 2) the file was empty before and is still empty; 3) the file has been deleted. Perhaps this is a bug in scm-record and it should return SelectedContents::Unchanged or SelectedContents::Present if the file hasn't been deleted. Until this is patched upstream, we can work around it by disambiguating these cases.

See arxanas/scm-record#26 for the upstream bug.

Fixes #3016

@emesterhazy
Copy link
Contributor Author

This is a first pass at fixing #3016. The upstream scm-diff-editor binary has the same UI issue, and I think there is also an upstream bug with the way File::get_selected_contents interacts with empty files.

This PR should be a complete fix for jj. I'd prefer to merge this first and fix the upstream after in collaboration with @arxanas. If we change the File::get_selected_contents then we can simplify the logic in jj in a follow-up PR.

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but @arxanas might have better suggestion.

cli/src/merge_tools/builtin.rs Show resolved Hide resolved
cli/src/merge_tools/builtin.rs Show resolved Hide resolved
@emesterhazy emesterhazy force-pushed the push-zlpuryytotqv branch 2 times, most recently from fef2509 to ef95706 Compare April 14, 2024 13:55
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. There are no other comments, so I went ahead and flagged as approved.

cli/src/merge_tools/builtin.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@arxanas arxanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing!

cli/src/merge_tools/builtin.rs Show resolved Hide resolved
cli/src/merge_tools/builtin.rs Outdated Show resolved Hide resolved
cli/src/merge_tools/builtin.rs Show resolved Hide resolved
@emesterhazy
Copy link
Contributor Author

Thanks for reviewing @arxanas!

@emesterhazy
Copy link
Contributor Author

emesterhazy commented Apr 16, 2024

I'm going to submit this now and work on a patch for scm-record upstream. Once that's done I'll open another PR to incorporate the upstream changes.

@martinvonz
Copy link
Member

martinvonz commented Apr 16, 2024

Sounds good. Thanks, both of you! EDIT: all three of you!

This fixes several issues that made working with empty files difficult using
the builtin diff editor.

1. The scm-record library used for the editor expects each file to have at
least one section. For new empty files this should be a file mode section. jj
wasn't rendering this mode section, which prevented empty files from being
selected at all.

2. The scm-record library returns `SelectedContents::Absent` if the file has no
contents after the diff editor is invoked. This can be because of several
reasons: 1) the file is new and empty; 2) the file was empty before and is
still empty; 3) the file has been deleted. Perhaps this is a bug in scm-record
and it should return `SelectedContents::Unchanged` or
`SelectedContents::Present` if the file hasn't been deleted. Until this is
patched upstream, we can work around it by disambiguating these cases.

See arxanas/scm-record#26 for the upstream bug.


Fixes #3016
@emesterhazy emesterhazy merged commit fc49d35 into main Apr 16, 2024
16 checks passed
@emesterhazy emesterhazy deleted the push-zlpuryytotqv branch April 16, 2024 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty files in jj split
4 participants