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.

Sign up for GitHub

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

diffedit: start the builtin ui with all boxes checked #5393

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

diffedit: start the builtin ui with all boxes checked #5393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
[streampager](https://github.com/markbt/streampager/). It can handle large
inputs better.

* The terminal UI of `jj diffedit` now starts with the whole diff checked, meaning
you need to uncheck what you want to discard, instead of checking what you want to
preserve.

### Deprecations

### New features
Expand Down
9 changes: 8 additions & 1 deletion cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ use crate::formatter::FormatRecorder;
use crate::formatter::Formatter;
use crate::formatter::PlainTextFormatter;
use crate::merge_tools::DiffEditor;
use crate::merge_tools::InitialSelection;
use crate::merge_tools::MergeEditor;
use crate::merge_tools::MergeToolConfigError;
use crate::operation_templater::OperationTemplateLanguage;
Expand Down Expand Up @@ -2950,7 +2951,13 @@ impl DiffSelector {
// whereas we want to update the left tree. Unmatched paths
// shouldn't be based off the right tree.
let right_tree = right_tree.store().get_root_tree(&selected_tree_id)?;
Ok(editor.edit(left_tree, &right_tree, matcher, format_instructions)?)
Ok(editor.edit(
left_tree,
&right_tree,
matcher,
format_instructions,
InitialSelection::None,
Comment on lines +2958 to +2959
Copy link
Contributor

Choose a reason for hiding this comment

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

comment (non-blocking): We could consider if we want to bundle the "configuration" kind of arguments here into a single struct for organizational reasons, but I don't feel strongly.

)?)
}
}
}
Expand Down
9 changes: 8 additions & 1 deletion cli/src/commands/diffedit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use crate::cli_util::CommandHelper;
use crate::cli_util::RevisionArg;
use crate::command_error::CommandError;
use crate::complete;
use crate::merge_tools::InitialSelection;
use crate::ui::Ui;

/// Touch up the content changes in a revision with a diff editor
Expand Down Expand Up @@ -132,7 +133,13 @@ don't make any changes, then the operation will be aborted.",
};
let base_tree = merge_commit_trees(tx.repo(), base_commits.as_slice())?;
let tree = target_commit.tree()?;
let tree_id = diff_editor.edit(&base_tree, &tree, &EverythingMatcher, format_instructions)?;
let tree_id = diff_editor.edit(
&base_tree,
&tree,
&EverythingMatcher,
format_instructions,
InitialSelection::All,
)?;
if tree_id == *target_commit.tree_id() {
writeln!(ui.status(), "Nothing changed.")?;
} else {
Expand Down
2 changes: 2 additions & 0 deletions cli/src/commands/unsquash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::cli_util::RevisionArg;
use crate::command_error::user_error;
use crate::command_error::CommandError;
use crate::description_util::combine_messages;
use crate::merge_tools::InitialSelection;
use crate::ui::Ui;

/// Move changes from a revision's parent into the revision
Expand Down Expand Up @@ -106,6 +107,7 @@ aborted.
&parent_tree,
&EverythingMatcher,
format_instructions,
InitialSelection::None,
)?;
if new_parent_tree_id == parent_base_tree.id() {
return Err(user_error("No changes selected"));
Expand Down
71 changes: 59 additions & 12 deletions cli/src/merge_tools/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,14 +233,30 @@ fn read_file_contents(
}
}

#[derive(Clone, Copy, Debug)]
pub enum InitialSelection {
All,
None,
}

impl InitialSelection {
fn is_checked(&self) -> bool {
match self {
InitialSelection::All => true,
InitialSelection::None => false,
}
}
}

fn make_section_changed_lines(
contents: &str,
change_type: scm_record::ChangeType,
initial_selection: InitialSelection,
) -> Vec<scm_record::SectionChangedLine<'static>> {
contents
.split_inclusive('\n')
.map(|line| scm_record::SectionChangedLine {
is_checked: false,
is_checked: initial_selection.is_checked(),
change_type,
line: Cow::Owned(line.to_owned()),
})
Expand All @@ -250,6 +266,7 @@ fn make_section_changed_lines(
fn make_diff_sections(
left_contents: &str,
right_contents: &str,
initial_selection: InitialSelection,
) -> Result<Vec<scm_record::Section<'static>>, BuiltinToolError> {
let diff = Diff::by_line([left_contents.as_bytes(), right_contents.as_bytes()]);
let mut sections = Vec::new();
Expand Down Expand Up @@ -285,8 +302,16 @@ fn make_diff_sections(
})?;
sections.push(scm_record::Section::Changed {
lines: [
make_section_changed_lines(left_side, scm_record::ChangeType::Removed),
make_section_changed_lines(right_side, scm_record::ChangeType::Added),
make_section_changed_lines(
left_side,
scm_record::ChangeType::Removed,
initial_selection,
),
make_section_changed_lines(
right_side,
scm_record::ChangeType::Added,
initial_selection,
),
]
.concat(),
});
Expand All @@ -313,6 +338,7 @@ pub fn make_diff_files(
right_tree: &MergedTree,
changed_files: &[RepoPathBuf],
conflict_marker_style: ConflictMarkerStyle,
initial_selection: InitialSelection,
) -> Result<Vec<scm_record::File<'static>>, BuiltinToolError> {
let mut files = Vec::new();
for changed_path in changed_files {
Expand All @@ -323,7 +349,7 @@ pub fn make_diff_files(

if should_render_mode_section(&left_info, &right_info) {
sections.push(scm_record::Section::FileMode {
is_checked: false,
is_checked: initial_selection.is_checked(),
before: left_info.file_mode,
after: right_info.file_mode,
});
Expand Down Expand Up @@ -359,12 +385,16 @@ pub fn make_diff_files(
num_bytes: _,
},
) => sections.push(scm_record::Section::Changed {
lines: make_section_changed_lines(&contents, scm_record::ChangeType::Added),
lines: make_section_changed_lines(
&contents,
scm_record::ChangeType::Added,
initial_selection,
),
}),

(FileContents::Absent, FileContents::Binary { hash, num_bytes }) => {
sections.push(scm_record::Section::Binary {
is_checked: false,
is_checked: initial_selection.is_checked(),
old_description: None,
new_description: Some(Cow::Owned(describe_binary(hash.as_deref(), num_bytes))),
});
Expand All @@ -378,7 +408,11 @@ pub fn make_diff_files(
},
FileContents::Absent,
) => sections.push(scm_record::Section::Changed {
lines: make_section_changed_lines(&contents, scm_record::ChangeType::Removed),
lines: make_section_changed_lines(
&contents,
scm_record::ChangeType::Removed,
initial_selection,
),
}),

(
Expand All @@ -393,7 +427,11 @@ pub fn make_diff_files(
num_bytes: _,
},
) => {
sections.extend(make_diff_sections(&old_contents, &new_contents)?);
sections.extend(make_diff_sections(
&old_contents,
&new_contents,
initial_selection,
)?);
}

(
Expand All @@ -416,7 +454,7 @@ pub fn make_diff_files(
num_bytes: new_num_bytes,
},
) => sections.push(scm_record::Section::Binary {
is_checked: false,
is_checked: initial_selection.is_checked(),
old_description: Some(Cow::Owned(describe_binary(
old_hash.as_deref(),
old_num_bytes,
Expand All @@ -429,7 +467,7 @@ pub fn make_diff_files(

(FileContents::Binary { hash, num_bytes }, FileContents::Absent) => {
sections.push(scm_record::Section::Binary {
is_checked: false,
is_checked: initial_selection.is_checked(),
old_description: Some(Cow::Owned(describe_binary(hash.as_deref(), num_bytes))),
new_description: None,
});
Expand Down Expand Up @@ -528,6 +566,7 @@ pub fn edit_diff_builtin(
right_tree: &MergedTree,
matcher: &dyn Matcher,
conflict_marker_style: ConflictMarkerStyle,
initial_selection: InitialSelection,
) -> Result<MergedTreeId, BuiltinToolError> {
let store = left_tree.store().clone();
// TODO: handle copy tracking
Expand All @@ -542,6 +581,7 @@ pub fn edit_diff_builtin(
right_tree,
&changed_files,
conflict_marker_style,
initial_selection,
)?;
let mut input = scm_record::helpers::CrosstermInput;
let recorder = scm_record::Recorder::new(
Expand Down Expand Up @@ -622,8 +662,11 @@ fn make_merge_sections(
item: "conflicting hunk",
}
})?;
let changed_lines =
make_section_changed_lines(contents, change_type);
let changed_lines = make_section_changed_lines(
contents,
change_type,
InitialSelection::None,
);
Ok(changed_lines)
})
.flatten_ok()
Expand Down Expand Up @@ -731,6 +774,7 @@ mod tests {
&right_tree,
&changed_files,
ConflictMarkerStyle::Diff,
InitialSelection::None,
)
.unwrap();
insta::assert_debug_snapshot!(files, @r###"
Expand Down Expand Up @@ -868,6 +912,7 @@ mod tests {
&right_tree,
&changed_files,
ConflictMarkerStyle::Diff,
InitialSelection::None,
)
.unwrap();
insta::assert_debug_snapshot!(files, @r###"
Expand Down Expand Up @@ -939,6 +984,7 @@ mod tests {
&right_tree,
&changed_files,
ConflictMarkerStyle::Diff,
InitialSelection::None,
)
.unwrap();
insta::assert_debug_snapshot!(files, @r###"
Expand Down Expand Up @@ -1011,6 +1057,7 @@ mod tests {
&right_tree,
&changed_files,
ConflictMarkerStyle::Diff,
InitialSelection::None,
)
.unwrap();
insta::assert_debug_snapshot!(files, @r###"
Expand Down
16 changes: 10 additions & 6 deletions cli/src/merge_tools/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ use thiserror::Error;
use self::builtin::edit_diff_builtin;
use self::builtin::edit_merge_builtin;
use self::builtin::BuiltinToolError;
pub use self::builtin::InitialSelection;
pub(crate) use self::diff_working_copies::new_utf8_temp_dir;
use self::diff_working_copies::DiffCheckoutError;
use self::external::edit_diff_external;
Expand Down Expand Up @@ -255,14 +256,17 @@ impl DiffEditor {
right_tree: &MergedTree,
matcher: &dyn Matcher,
format_instructions: impl FnOnce() -> String,
initial_selection: InitialSelection,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to always open builtin editor with everything checked?

It's weird that initial_selection applies only to the builtin editor. External editor opens with the right content (i.e. everything selected?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion myself, but the per-command difference was discussed here: #5390 (comment)

It seems to make sense to me that we would want e.g. split to start with nothing but diffedit to start with everything. Maybe it's more of a limitation with external diff editors? Is there a way to indicate to an external diff editor "here is a set of changes, but don't apply any of them by default"?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's a way of telling external 2-way diff editors that. But we can tell external 3-way diff editors that by setting the middle state to be either the same as the left (for split) or the same as the right (for diffedit).

By the way, another aspect is what the effect of starting the diff editor and immediately quitting should do. One could argue that it should have the same effect as not even starting it. split and diffedit are always interactive (unless you pass paths to split), but squash and restore can be non-interactive. Is it important to us that jj squash has the same effect as running jj squash -i and immediately closing the diff editor? I'm fine with that inconsistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

another aspect is what the effect of starting the diff editor and immediately quitting should do.

Good point. I slightly prefer that -i (with noop tool) works in the same way as non-interactive edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't used external diff tools, so trying meld and meld-3 just now, they apparently start with everything selected (whereas the builtin ui starts with nothing selected). On the face of it, the inconsistency is a bit weird.

As Martin says, for 3-way diff editors, you could change this by choosing the middle file. For 2-way diff editors, you could change the default by swapping left and right, although that'd possibly be too confusing to use.

If we make the diff tools consistent, the most disrupting change would probably be jj commit -i. Thinking about it, I don't see a strong reason to prefer starting from the empty diff vs the working copy diff. It seems to be a question of habit.
In this case, git commit --interactive doesn't really set expectations one way or the other (not sure it's used much, as it's quite strange). With magit, you create commits from the empty diff, but it's something you do before calling the git commit equivalent, so it's like using git add -p + git commit, it doesn't seem quite comparable.

No clear conclusion.

) -> Result<MergedTreeId, DiffEditError> {
match &self.tool {
MergeTool::Builtin => {
Ok(
edit_diff_builtin(left_tree, right_tree, matcher, self.conflict_marker_style)
.map_err(Box::new)?,
)
}
MergeTool::Builtin => Ok(edit_diff_builtin(
left_tree,
right_tree,
matcher,
self.conflict_marker_style,
initial_selection,
)
.map_err(Box::new)?),
MergeTool::External(editor) => {
let instructions = self.use_instructions.then(format_instructions);
edit_diff_external(
Expand Down