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

Adjust get_selected_contents behavior #62

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

Conversation

30350n
Copy link

@30350n 30350n commented Jul 12, 2024

Draft PR to enable the fix for jj #3702 and jj #3846 in jj #4078.

The logic in get_selected_contents generally seems really confusing to me and these changes probably don't help mitigating that. From what I can tell, it doesn't break existing tests though, neither in this repo, nor in jj (in combination with jj #4078).

The main problems seem to be:

  1. The general structure of get_selected_contents
    Going through the sections one-by-one makes it hard to differentiate between Absent and Unchanged SelectedContents.
  2. The way scm-record handles (doesn't handle) file mode changes
    I feel like the client shouldn't be responsible for manually adding FileMode sections at all. Ideally this part wouldn't be exposed at all, but just be handled by scm-record internally.

- only return selected = 'Absent' if the file actually doesn't exist
  (return selected = 'Present' or 'Unchanged' if it's empty)
- only return unselected = 'Absent' if there is a unselected file mode
  change, changing the file mode to 'Absent'
@arxanas
Copy link
Owner

arxanas commented Jul 21, 2024

Going through the sections one-by-one makes it hard to differentiate

I agree that the file mode is treated weirdly and interacts poorly with the rest of the context/data model But rather than extend the logic, as done in this PR, I think we should remove it and simply report a file mode transition (selected Section::FileMode) to the caller, who can then decide how to handle it:

  • We should delete File::file_mode.
  • We should just return the file mode transition (if any) directly as a part of SelectedContents, and remove any logic that sets SelectedContents::Absent/SelectedContents::Present based on the file mode.
    • The caller can decide how to handle the file mode transition, if any.
    • There shouldn't be any special handling for FileMode::absent() when we're done.

I feel like the client shouldn't be responsible for manually adding FileMode sections at all. Ideally this part wouldn't be exposed at all, but just be handled by scm-record internally.

There are already some competing designs (such as how to handle partially-committed changes in conjunction with file mode changes), which indicates that scm-record might not be able to establish an authoritative design that works for all callers. Therefore, I think that scm-record should focus on rendering the UI and leave the semantic interpretation to the caller. (In accordance with that, we should also move get_selected_contents to the helpers module, as it imposes a semantic interpretation on the UI contents.)

@arxanas
Copy link
Owner

arxanas commented Jul 21, 2024

We should just return the file mode transition (if any) directly as a part of SelectedContents

Or we can keep SelectedContents as it is, and just update get_file_mode to not check the File::file_mode, and have the caller continue to handle that separately.

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.

2 participants