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

Allow deleting files in diffedit and split #5486

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jgilchrist
Copy link
Contributor

@jgilchrist jgilchrist commented Jan 27, 2025

Extends #4078 but with a slightly different approach based on a discussion in Discord: https://discord.com/channels/968932220549103686/1325114717387100201

It relies on changes in arxanas/scm-record#93 to function.

The code in this area gets significantly simpler, since there's no longer any ambiguity to resolve. The scm-record changes expect that any mode changes are represented by file mode sections, and adds UI hooks to simplify working with them (e.g. automatically de-selecting a deletion mode change if any lines in the file are de-selected, since the file still needs to exist to hold those lines).

As a result, mode changes always come back from scm-record as a FileModeTransition and we can handle them explicitly. 'No changes selected' always comes back as None, so if we get None we can always skip the file entirely and use the previous content/mode.

This also uses code from #4078 to ensure executable bits are respected.

Fixes #3702
Fixes #3846.

Checklist

If applicable:

  • I have updated CHANGELOG.md (I will once it's confirmed that everyone is happy with the approach)
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@jgilchrist jgilchrist force-pushed the push-oxwvtzvtqrmu branch 6 times, most recently from e22b6f0 to 7ac982e Compare January 27, 2025 14:29
@jgilchrist jgilchrist marked this pull request as draft January 27, 2025 15:41
@jgilchrist jgilchrist marked this pull request as ready for review January 28, 2025 11:02
@jgilchrist jgilchrist force-pushed the push-oxwvtzvtqrmu branch 2 times, most recently from 347f2d4 to 3537330 Compare January 28, 2025 11:06
@PhilipMetzger
Copy link
Contributor

The commit style still applies, so the relevant (last) commit probably should use something like builtin: as a topic.

@jgilchrist
Copy link
Contributor Author

@PhilipMetzger Thanks, I wasn't sure what to use as the topic. Prior commits in the history seem to use diff-editor, I've gone with that since I'm not sure whether there are other things builtin could refer to (pager?). If you'd prefer builtin though I'll change it.

@PhilipMetzger
Copy link
Contributor

@PhilipMetzger Thanks, I wasn't sure what to use as the topic. Prior commits in the history seem to use diff-editor, I've gone with that since I'm not sure whether there are other things builtin could refer to (pager?). If you'd prefer builtin though I'll change it.

SGTM, no need for a further change as my suggestion probably was misleading.

@arxanas arxanas added the scm-record Issues relating to the scm-record library, used as the default interactive diff/merge editor. label Jan 29, 2025
…iles

The code in this area gets significantly simpler, since there's no
longer any ambiguity to resolve. The scm-record changes expect that any
mode changes are represented by file mode sections, and adds UI hooks to
simplify working with them (e.g. automatically de-selecting a deletion
mode change if any lines in the file are de-selected, since the file
still needs to exist to hold those lines).

As a result, mode changes always come back from scm-record as a
FileModeTransition and we can handle them explicitly. 'No changes
selected' always comes back as None, so if we get None we can always
skip the file entirely and use the previous content/mode.
@martinvonz
Copy link
Member

@arxanas, did you say that you're going to review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scm-record Issues relating to the scm-record library, used as the default interactive diff/merge editor.
Projects
None yet
4 participants