Skip to content

cli: merge_tools/external: bail out if the files are the same when re-reading conflicts - #9984

Open
winterqt wants to merge 1 commit into
mainfrom
push-smnstoxmwxmv
Open

cli: merge_tools/external: bail out if the files are the same when re-reading conflicts#9984
winterqt wants to merge 1 commit into
mainfrom
push-smnstoxmwxmv

Conversation

@winterqt

Copy link
Copy Markdown
Member

Previously, we were always updating the tree, even if the files were unchanged by the external editor.

Fixes #9138.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added/updated tests to cover my changes
  • I fully understand the code that I am submitting (what it does,
    how it works, how it's organized), including any code drafted by an LLM.
  • For any prose generated by an LLM, I have proof-read and copy-edited with
    an eye towards deleting anything that is irrelevant, clarifying anything
    that is confusing, and adding details that are relevant. This includes,
    for example, commit descriptions, PR descriptions, and code comments.

@winterqt
winterqt requested a review from a team as a code owner August 14, 2026 01:05
…-reading conflicts

Previously, we were always updating the tree, even if the files were unchanged by the external editor.

Fixes #9138.
};

if new_file_ids == file.unsimplified_ids {
return Err(ConflictResolveError::EmptyOrUnchanged);

@winterqt winterqt Aug 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I kind of want to use a more specific error here (that is, definitively unchanged, not "unchanged or empty"). Happy to make that error and use it instead of this if we want.

Merge::normal(new_file_id)
};

if new_file_ids == file.unsimplified_ids {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Can you move this into the if editor.merge_tool_edits_conflict_markers || exit_status_implies_conflict block? It only matters when non-identical materialized content can produce the same output.

std::fs::write(&editor_script, "")?;
let output = work_dir.run_jj([
"resolve",
"--config=merge-tools.fake-editor.merge-tool-edits-conflict-markers=true",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to test the fixed issue. We need to modify the materialized content.

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.

jj resolve always creates new op, even if no conflicts were resolved

2 participants