cli: resolve: do not use .with_new_file_ids() to construct resolved file value #6288
Conversation
There was a problem hiding this comment.
I think we should instead make it an error to try to resolve the content-level conflict before resolving the executable-bit-level conflict. I think we should do the same when merging copy records, i.e. we should not pick one side but instead make the user decide which copy id to keep.
Just to clarify, instead of using |
|
Yes. I think it's fine to use |
I think the point of using |
…lved This reverts a01d0bf "cli: resolve: leave executable bit unchanged when using external tool", and adds test that would panic otherwise. As Martin said, "we should instead make it an error to try to resolve the content-level conflict before resolving the executable-bit-level conflict. I think we should do the same when merging copy records." jj-vcs#6288 (review) Since exec bit has only two states, it can always be trivially resolved. We'll need to think about the UI when we add CopyId handling.
yuja
changed the title
This seems a bit easier to follow than nesting Option types. try_resolve_file_conflict() doesn't have this problem, but it is the only caller of maybe_map(), so I've gone ahead and replaced it, too. The return type is unchanged because it looked equally bad if to_tree_merge() returned Result<Result<Merge<Tree>, ()>, BackendError>.
…olved As Martin suggested, "we should instead make it an error to try to resolve the content-level conflict before resolving the executable-bit-level conflict. I think we should do the same when merging copy records." jj-vcs#6288 (review)
We're going to make "jj resolve" error out on exec bit conflict. This behavior should be more consistent with tree::try_resolve_file_conflict(), which exits early if the tree value had absent entries. Since unchanged exec bit shouldn't be lost when resolving change-delete content conflict, the resolution function falls back to the original state if the exec-bit conflict is resolved to None.
…olved As Martin suggested, "we should instead make it an error to try to resolve the content-level conflict before resolving the executable-bit-level conflict. I think we should do the same when merging copy records." jj-vcs#6288 (review)
…ile value This reverts a01d0bf "cli: resolve: leave executable bit unchanged when using external tool", and updates handling of resolved content. Fixes jj-vcs#6250
…olved As Martin suggested, "we should instead make it an error to try to resolve the content-level conflict before resolving the executable-bit-level conflict. I think we should do the same when merging copy records." #6288 (review)
Fixes #6250
Checklist
If applicable:
CHANGELOG.md