Move file mode transitions into SelectedContents

[jgilchrist]

Based on #62 (comment) as an attempt to solve the same issue.

@arxanas - wanted to get this up sooner rather than later to get feedback on whether it looks like I'm going in the same direction you were thinking here. I intentionally have not put a lot of thought, yet, into naming, docs, etc.

The main changes are:

• Removes File::file_mode
• Introduced another struct above SelectedChanges (currently called FileState which feels like a terrible name) to capture whether the file in question is absent or present. If it is present, we have the existing SelectedChanges plus an extra field which records a possible (Optional) file mode transition, along with the existing SelectedChanges.
• Adds set_mode_change to this struct and use it depending on the selected state of any Section::FileMode sections. We also need get_mode_change to ensure that the transition is preserved for Section::Binary

[jgilchrist]

In theory I suppose it would be possible to print something about the mode change now, but I've left it out

[jgilchrist]

I might be missing something as it doesn't look like changes to the file's mode are handled as part of apply_changes

[jgilchrist]

Diff renders much nicer with whitespace comparisons disabled

[jgilchrist]

jj-vcs/jj#5481 has the corresponding changes in jj. Although it fixes the deletion behaviour, unfortunately it regresses the behaviour around empty files, which was addressed in the original PR here.

I'm not sure what's best here. I was thinking it would be possible to reproduce the other PR's behaviour of returning Unchanged for empty files rather than Absent by scanning the list of Sections to see if there are mode changes and using that to determine the return value. But that goes slightly against your comment where you suggested moving away from assigning meaning to any sections and rather just reporting back what happened in the UI. Going that direction complicates things because we no longer have file_mode to go off, so it's the caller who ends up having to do the disambiguation, but also now needs to keep track of the file mode since they can't use the mode from File anymore.

Not sure what the best path forwards is here.

[30350n]

I'm not sure what I think of the FileState struct tbh, it seems kind of unnecessary.

From what I've understood since writing my original two draft PRs, @arxanas basically intended scm-record to work in a way where it always receives a separate FileMode section. The main problem is that jj omits this for newly created files with contents, nor deleted files that previously had contents.

If jj would properly provide this all the time, we could simply get rid of File::file_mode and won't require your additional FileState struct either (I think).

To make this actually feel good in use then, we'd have to add some hooks to the selection part of scm_record which automatically deselect/select changes in certain scenarios. We already discussed that in a thread on the jj discord a while ago, here's the relevant excerpt from that:

bnjmnt4n:
Based on reading the various issues, I'm of the impression that we should follow the suggestions recommended by #26 (comment) and #62 (comment), to always render a filemode section for all files which are newly created/deleted/had their file mode changed. This avoids the ambiguity between e.g. deleting a file (requires selecting the filemode change and selecting all line deletions) and truncating a file only (requires selecting all line deletions). However, there is a new ambiguous state which is when the filemode change is selected, but only some of the line deletions are selected. In this case, we could just delete the selected lines without deleting the whole file (whilst printing a warning).

bnjmnt4n:
I think any alternative would probably require wholesale changes to scm-record.
Looking at jj-vcs/jj#5189 as well, for the best UX, we might need to add some logic hooks to scm-record (e.g. prevent selecting of a given section when another section is selected)

Bobbe (me):
i think i generally agree, but there are a lots of small nuances to make this feel good (QOL wise)
for example regarding this:
> However, there is a new ambiguous state which is when the filemode change is selected, but only some of the line deletions are selected.
instead of your suggestion (printing a warning), i'd rather make it so that
a) when you select a deletion filemode change, all lines get automatically selected
and maybe b) when you select all lines and there is a deletion file mode change, it also get automatically selected (although the user should be able to manually deselect it again)
similar behaviors appear in the file creation state:
a) if there's a creation filemode change, selecting any line should automatically select it
b) if you manually deselect all lines again, the filemode change should also be deselected

bnjmnt4n:
yeah I agree with your suggestion, but I'm guessing it might be a bit more work adding these hooks into scm-record. so my suggestion is kind of the minimum to get this working
Would love to get input from arxanas as well.

Bobbe (me):
scm-record probably has to be touched regardless, so we might as well do things properly imo

[jgilchrist]

I'm not sure what I think of the FileState struct tbh, it seems kind of unnecessary.

The idea was that you'd want to report the selection of a FileMode change back to the caller, so you can handle creation/deletion of empty files, since in this case the only section is a mode change. If you want to do that, it needs to be part of the return value of get_selected_changes() somehow - I think this was alluded to in #62 (comment) but I don't know if that precedes the discussion you quoted.

Either way, on the assumption that you do want the FileMode change to be returned somehow from get_selected_changes, we need to figure out how it fits into the return value. You could add a mode_transition to each of the SelectedContents variants, but it didn't quite feel right to be for it to be 'below' the contents since a mode change feels independent from the contents of the file. So I went with a struct that makes the separation explicit - scm-record may report back some changes to the file, and/or a change to the file's mode.

If jj would properly provide this all the time, we could simply get rid of File::file_mode and won't require your additional FileState struct either (I think).

Perhaps, although you presumably still need some way to signal whether the section for the file mode change was selected or not, particularly for cases where the mode change can be selected independently of contents changes (e.g. exec bit flips).

To make this actually feel good in use then, we'd have to add some hooks to the selection part of scm_record which automatically deselect/select changes in certain scenarios.

I definitely like the approach discussed here, especially because the resulting logic in jj after this PR is still a bit of a headache as it still has to deal with the ambiguity mentioned here. I'm happy to have a go at implementing it that way, unless it's felt that the approach here is enough of an improvement on top of what exists that it's worth moving forwards with as a stepping stone to that eventual design.

[30350n]

The idea was that you'd want to report the selection of a FileMode change back to the caller, so you can handle creation/deletion of empty files, since in this case the only section is a mode change. If you want to do that, it needs to be part of the return value of get_selected_changes() somehow

I think we should simply also always call get_file_mode:

scm-record/scm-record/src/types.rs

Lines 241 to 244 in c23e319

	/// Get the new Unix file mode. If the user selected a
	/// [`Section::FileMode`], then returns that file mode. Otherwise, returns
	/// the `file_mode` value that this [`File`] was constructed with.
	pub fn get_file_mode(&self) -> Option<FileMode> {

for each file in apply_diff_builtin on the jj side as well.

I guess to expand on what I mentioned earlier:

The main problem is that jj omits this for newly created files with contents, nor deleted files that previously had contents ...

... and seemingly also doesn't handle any selected FileMode sections.

I think we should a) remove File::file_mode, b) rename File.get_file_mode to File.get_file_mode_change or File.get_selected_file_mode_change or something, and c) (on the jj side) always include a FileMode section if the filemode changed in make_diff_files and always handle any selected file mode changes by calling File.get_selected_file_mode_change in apply_diff_builtin.

For completness sake:

Perhaps, although you presumably still need some way to signal whether the section for the file mode change was selected or not, particularly for cases where the mode change can be selected independently of contents changes (e.g. exec bit flips).

Yes, exactly, I think (though not 100% sure) that's what File.get_file_mode is meant for already, thus I wrote what I wrote above ^^

[jgilchrist]

@30350n Got you, thanks for the explanation. The only advantage I can see to maintaining the approach I've taken here is that it's harder to forget to call to handle the file mode changes. For example, while fixing up call sites that handled get_selected_changes I had to add mode_transition to various other places, each of which is going to need to change its handling since deletions are no longer signalled via Absent but rather by mode_transition: (x -> 0). IMO it's quite nice that other scm-record consumers might get similar compile errors that force them to check if they need to handle mode transitions.

In a draft of the changes I've got going, the return value of get_selected_changes is (Option<SelectedElements>, Option<SelectedElements>) where SelectedElements is:

#[derive(Debug)]
pub struct SelectedElements<'a> {
    /// A potential change to the file's mode.
    pub mode_transition: Option<FileModeTransition>,

    /// The file's contents.
    pub contents: SelectedContents<'a>,
}

Thus if nothing was selected at all for a file, we return None.

If only a mode transition was performed, we return:

SelectedElements {
    mode_transition: Some(transition),

    contents: SelectedContents::Unchanged,
}

If a mode transition (e.g. exec bit flip) was performed in combination with a contents change, we return:

SelectedElements {
    mode_transition: Some(transition),

    contents: SelectedContents::Text { ... },
}

Once I've got the corresponding jj changes, I'll file a separate PR so it's easier to compare.

[jgilchrist]

Closing this in lieu of #93